From d3c34c9df728281ffdc586eb00300d36c71c6d7d Mon Sep 17 00:00:00 2001 From: Anton Eliasson Date: Sun, 2 Oct 2016 14:59:20 +0200 Subject: [PATCH 1/4] Reflow code --- .../org/lcdproc/lcdjava/LCDSocketPoller.java | 63 +++++++------------ 1 file changed, 23 insertions(+), 40 deletions(-) diff --git a/src/main/java/org/lcdproc/lcdjava/LCDSocketPoller.java b/src/main/java/org/lcdproc/lcdjava/LCDSocketPoller.java index 6f1233b..ca95c96 100644 --- a/src/main/java/org/lcdproc/lcdjava/LCDSocketPoller.java +++ b/src/main/java/org/lcdproc/lcdjava/LCDSocketPoller.java @@ -11,16 +11,16 @@ /** * Thread that listens for data on the LCD socket. *

Copyright (c) 2004-2005 Darren Greaves. + * * @author Darren Greaves * @version $Id: LCDSocketPoller.java,v 1.3 2008-07-06 15:38:34 boncey Exp $ */ -public class LCDSocketPoller extends Thread -{ +public class LCDSocketPoller extends Thread { /** * Version details. */ public static final String CVSID = - "$Id: LCDSocketPoller.java,v 1.3 2008-07-06 15:38:34 boncey Exp $"; + "$Id: LCDSocketPoller.java,v 1.3 2008-07-06 15:38:34 boncey Exp $"; private static Logger _log = LoggerFactory.getLogger(LCDSocketPoller.class); @@ -59,12 +59,12 @@ public class LCDSocketPoller extends Thread /** * Public constructor. - * @param in the BufferedReader that will recieve data from the server. + * + * @param in the BufferedReader that will recieve data from the server. * @param listener the LCDListener that gets notified of screens being - * listened to or ignored. + * listened to or ignored. */ - public LCDSocketPoller(BufferedReader in, LCDListener listener) - { + public LCDSocketPoller(BufferedReader in, LCDListener listener) { _in = in; _listener = listener; } @@ -73,56 +73,39 @@ public LCDSocketPoller(BufferedReader in, LCDListener listener) * Poll the socket every 100 milliseconds. */ @Override - public void run() - { - while (!interrupted()) - { + public void run() { + while (!interrupted()) { try { - if (!_in.ready()) { - Thread.sleep(POLL); - } - if (_in.ready()) - { + if (_in.ready()) { _lastLine = _in.readLine(); if (_lastLine == null) continue; Matcher listenIgnore = IGNORE_STATUS.matcher(_lastLine); Matcher menuEvent = MENU_STATUS.matcher(_lastLine); - if (_lastLine.startsWith(LCD.RESPONSE_ERROR)) - { + if (_lastLine.startsWith(LCD.RESPONSE_ERROR)) { _log.warn("Got a response of " + _lastLine + " from server"); - } - else if (_listener != null) - { + } else if (_listener != null) { - if (listenIgnore.matches()) - { + if (listenIgnore.matches()) { boolean listen = (LCD.RESPONSE_LISTEN.equals( - listenIgnore.group(1))); - try - { + listenIgnore.group(1))); + try { int screenId = Integer.parseInt(listenIgnore.group(2)); _listener.setListenStatus(screenId, listen); - } - catch (NumberFormatException e) - { + } catch (NumberFormatException e) { // Ignore } - } - else if (menuEvent.matches()) - { + } else if (menuEvent.matches()) { _listener.menuAction(menuEvent.group(2), menuEvent.group(1), menuEvent.group(3)); } } + } else { + Thread.sleep(POLL); } - } - catch (InterruptedException e) - { + } catch (InterruptedException e) { interrupt(); - } - catch (IOException e) - { + } catch (IOException e) { _log.error("Caught IOException", e); } } @@ -133,10 +116,10 @@ else if (menuEvent.matches()) /** * Get the last line received non-blocking. *

Calling this clears the last line received. + * * @return the last line received. */ - synchronized String getLastLine() - { + synchronized String getLastLine() { String ret = _lastLine; _lastLine = null; return ret; From f8e532e61bf4f12279e4ca492d34f6ea83938c28 Mon Sep 17 00:00:00 2001 From: Anton Eliasson Date: Sun, 2 Oct 2016 20:25:35 +0200 Subject: [PATCH 2/4] Rewrite LCDSocketPoller to use blocking I/O instead of polling This made the shutdown procedure a little more interesting since BufferedReader's read(Line) cannot easily be interrupted. One possible solution is to use shutdownInput() on the socket to provoke an End of Stream [1], which is what is done here. The other way is to just close the socket, causing an IOException. I opted not to do that since I would have to suppress all IOExceptions in basically the entire run() method in LCDSocketPoller [1]: https://stackoverflow.com/questions/3595926/how-to-interrupt-bufferedreaders-readline --- src/main/java/org/lcdproc/lcdjava/LCD.java | 12 ++-- .../org/lcdproc/lcdjava/LCDSocketPoller.java | 55 ++++++------------- 2 files changed, 24 insertions(+), 43 deletions(-) diff --git a/src/main/java/org/lcdproc/lcdjava/LCD.java b/src/main/java/org/lcdproc/lcdjava/LCD.java index 1d72e4a..b1a5880 100644 --- a/src/main/java/org/lcdproc/lcdjava/LCD.java +++ b/src/main/java/org/lcdproc/lcdjava/LCD.java @@ -456,25 +456,27 @@ public void menuAction(String menuId, String eventType, String value) { * Shut down the server, terminating any threads. * @throws LCDException in case of a network problem. */ + @SuppressWarnings("WeakerAccess") public void shutdown() throws LCDException { _log.debug("Shutdown requested"); if (_poller != null) { - _poller.interrupt(); - _log.debug("Waiting for LCDSocketPoller to terminate..."); try { + _log.debug("Carefully killing socket..."); + _socket.shutdownInput(); + _log.debug("Waiting for LCDSocketPoller to terminate..."); _poller.join(); - } catch (InterruptedException e) { - e.printStackTrace(); + } catch (InterruptedException | IOException e) { + _log.error(e.toString()); } } try { _log.debug("Closing socket"); - if (_socket != null && !_socket.isClosed()) + if (_socket != null) { _socket.close(); } diff --git a/src/main/java/org/lcdproc/lcdjava/LCDSocketPoller.java b/src/main/java/org/lcdproc/lcdjava/LCDSocketPoller.java index ca95c96..ba25745 100644 --- a/src/main/java/org/lcdproc/lcdjava/LCDSocketPoller.java +++ b/src/main/java/org/lcdproc/lcdjava/LCDSocketPoller.java @@ -24,11 +24,6 @@ public class LCDSocketPoller extends Thread { private static Logger _log = LoggerFactory.getLogger(LCDSocketPoller.class); - /** - * How often to poll for changes. - */ - private static final int POLL = 100; - /** * The Pattern that matches ignore/listen events. */ @@ -74,42 +69,26 @@ public LCDSocketPoller(BufferedReader in, LCDListener listener) { */ @Override public void run() { - while (!interrupted()) { - try { - if (_in.ready()) { - _lastLine = _in.readLine(); - if (_lastLine == null) - continue; - Matcher listenIgnore = IGNORE_STATUS.matcher(_lastLine); - Matcher menuEvent = MENU_STATUS.matcher(_lastLine); - if (_lastLine.startsWith(LCD.RESPONSE_ERROR)) { - _log.warn("Got a response of " + _lastLine + - " from server"); - } else if (_listener != null) { - - if (listenIgnore.matches()) { - boolean listen = (LCD.RESPONSE_LISTEN.equals( - listenIgnore.group(1))); - try { - int screenId = Integer.parseInt(listenIgnore.group(2)); - _listener.setListenStatus(screenId, listen); - } catch (NumberFormatException e) { - // Ignore - } - } else if (menuEvent.matches()) { - _listener.menuAction(menuEvent.group(2), menuEvent.group(1), menuEvent.group(3)); - } - } - } else { - Thread.sleep(POLL); + try { + while ((_lastLine = _in.readLine()) != null) { + if (_lastLine.startsWith(LCD.RESPONSE_ERROR)) { + _log.warn("Got a response of " + _lastLine + + " from server"); + } + Matcher listenIgnore = IGNORE_STATUS.matcher(_lastLine); + Matcher menuEvent = MENU_STATUS.matcher(_lastLine); + if (listenIgnore.matches()) { + boolean listen = (LCD.RESPONSE_LISTEN.equals(listenIgnore.group(1))); + int screenId = Integer.parseInt(listenIgnore.group(2)); + _listener.setListenStatus(screenId, listen); + } else if (menuEvent.matches()) { + _listener.menuAction(menuEvent.group(2), menuEvent.group(1), menuEvent.group(3)); } - } catch (InterruptedException e) { - interrupt(); - } catch (IOException e) { - _log.error("Caught IOException", e); } + } catch (IOException e) { + _log.error("Caught IOException", e); + throw new LCDException(e); } - _log.debug("Terminating"); } From f3a414425b72c6447bc8b93bd975e33cf4b88172 Mon Sep 17 00:00:00 2001 From: Anton Eliasson Date: Sun, 2 Oct 2016 20:36:01 +0200 Subject: [PATCH 3/4] Cleanup. More final fields and remove unused stuff. --- .../org/lcdproc/lcdjava/LCDSocketPoller.java | 25 +++++-------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/src/main/java/org/lcdproc/lcdjava/LCDSocketPoller.java b/src/main/java/org/lcdproc/lcdjava/LCDSocketPoller.java index ba25745..078c753 100644 --- a/src/main/java/org/lcdproc/lcdjava/LCDSocketPoller.java +++ b/src/main/java/org/lcdproc/lcdjava/LCDSocketPoller.java @@ -10,19 +10,9 @@ /** * Thread that listens for data on the LCD socket. - *

Copyright (c) 2004-2005 Darren Greaves. - * - * @author Darren Greaves - * @version $Id: LCDSocketPoller.java,v 1.3 2008-07-06 15:38:34 boncey Exp $ */ -public class LCDSocketPoller extends Thread { - /** - * Version details. - */ - public static final String CVSID = - "$Id: LCDSocketPoller.java,v 1.3 2008-07-06 15:38:34 boncey Exp $"; - - private static Logger _log = LoggerFactory.getLogger(LCDSocketPoller.class); +class LCDSocketPoller extends Thread { + private final Logger _log = LoggerFactory.getLogger(LCDSocketPoller.class); /** * The Pattern that matches ignore/listen events. @@ -39,7 +29,7 @@ public class LCDSocketPoller extends Thread { /** * The Reader to read data from. */ - private BufferedReader _in; + private final BufferedReader _in; /** * The last line of data received. @@ -50,23 +40,20 @@ public class LCDSocketPoller extends Thread { /** * The listener to notify of listen/ignore events. */ - private LCDListener _listener; + private final LCDListener _listener; /** * Public constructor. * - * @param in the BufferedReader that will recieve data from the server. + * @param in the BufferedReader that will receive data from the server. * @param listener the LCDListener that gets notified of screens being * listened to or ignored. */ - public LCDSocketPoller(BufferedReader in, LCDListener listener) { + LCDSocketPoller(BufferedReader in, LCDListener listener) { _in = in; _listener = listener; } - /** - * Poll the socket every 100 milliseconds. - */ @Override public void run() { try { From a4fca135392a571d6b5742bd14a443c1662e18d3 Mon Sep 17 00:00:00 2001 From: Anton Eliasson Date: Wed, 5 Oct 2016 20:41:11 +0200 Subject: [PATCH 4/4] Communicate LCDd readiness with a Semaphore instead of polling Access to _lastLine is now properly synchronized. It is a bit ugly to use a field just to send back a status line once, but it seems the easiest way in the current implementation. It is also not customary to increment a semaphore more times than you plan on decrementing it, but it's not strictly illegal either. LCD::connect now only tries to connect once. Connection issues should be rare, and it is up to the application what to do if the first attempt should fail. --- src/main/java/org/lcdproc/lcdjava/LCD.java | 42 ++++++++----------- .../org/lcdproc/lcdjava/LCDSocketPoller.java | 22 ++++++---- 2 files changed, 32 insertions(+), 32 deletions(-) diff --git a/src/main/java/org/lcdproc/lcdjava/LCD.java b/src/main/java/org/lcdproc/lcdjava/LCD.java index b1a5880..1a91baa 100644 --- a/src/main/java/org/lcdproc/lcdjava/LCD.java +++ b/src/main/java/org/lcdproc/lcdjava/LCD.java @@ -9,6 +9,8 @@ import java.net.Socket; import java.util.HashMap; import java.util.Map; +import java.util.concurrent.Semaphore; +import java.util.concurrent.TimeUnit; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -627,38 +629,28 @@ private void parseInitString(String init) * @return the response from the server. * @throws IOException in case of a network problem. */ - private String connect(String host, int port, String clientName) - throws IOException - { + private String connect(String host, int port, String clientName) throws IOException { _socket = new Socket(host, port); BufferedReader in = new BufferedReader(new InputStreamReader( - _socket.getInputStream())); + _socket.getInputStream())); _out = new BufferedWriter(new OutputStreamWriter( - _socket.getOutputStream())); - _poller = new LCDSocketPoller(in, this); + _socket.getOutputStream())); + Semaphore serverHello = new Semaphore(1); + _poller = new LCDSocketPoller(in, this, serverHello); _poller.start(); - write(CMD_INIT); - String response = null; - - for (int i = 0; i < POLL_REPEAT && response == null; i++) - { - synchronized (this) - { - try - { - Thread.sleep(POLL); - response = _poller.getLastLine(); - } - catch (InterruptedException e) - { - // Do nothing - } + try { + if (serverHello.tryAcquire(5, TimeUnit.SECONDS)) { + String version = _poller.getLastLine(); + write(CMD_CLIENT_SET + clientName); + return version; + } else { + // Timeout => failed to connect + return null; } + } catch (InterruptedException e) { + return null; } - write(CMD_CLIENT_SET + clientName); - - return response; } /** diff --git a/src/main/java/org/lcdproc/lcdjava/LCDSocketPoller.java b/src/main/java/org/lcdproc/lcdjava/LCDSocketPoller.java index 078c753..ef8070d 100644 --- a/src/main/java/org/lcdproc/lcdjava/LCDSocketPoller.java +++ b/src/main/java/org/lcdproc/lcdjava/LCDSocketPoller.java @@ -2,6 +2,7 @@ import java.io.BufferedReader; import java.io.IOException; +import java.util.concurrent.Semaphore; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -42,6 +43,8 @@ class LCDSocketPoller extends Thread { */ private final LCDListener _listener; + private Semaphore hello; + /** * Public constructor. * @@ -49,21 +52,23 @@ class LCDSocketPoller extends Thread { * @param listener the LCDListener that gets notified of screens being * listened to or ignored. */ - LCDSocketPoller(BufferedReader in, LCDListener listener) { + LCDSocketPoller(BufferedReader in, LCDListener listener, Semaphore hello) throws IOException { _in = in; _listener = listener; + this.hello = hello; } @Override public void run() { try { - while ((_lastLine = _in.readLine()) != null) { - if (_lastLine.startsWith(LCD.RESPONSE_ERROR)) { - _log.warn("Got a response of " + _lastLine + + String line; + while ((line = _in.readLine()) != null) { + if (line.startsWith(LCD.RESPONSE_ERROR)) { + _log.warn("Got a response of " + line + " from server"); } - Matcher listenIgnore = IGNORE_STATUS.matcher(_lastLine); - Matcher menuEvent = MENU_STATUS.matcher(_lastLine); + Matcher listenIgnore = IGNORE_STATUS.matcher(line); + Matcher menuEvent = MENU_STATUS.matcher(line); if (listenIgnore.matches()) { boolean listen = (LCD.RESPONSE_LISTEN.equals(listenIgnore.group(1))); int screenId = Integer.parseInt(listenIgnore.group(2)); @@ -71,9 +76,12 @@ public void run() { } else if (menuEvent.matches()) { _listener.menuAction(menuEvent.group(2), menuEvent.group(1), menuEvent.group(3)); } + synchronized (this) { + _lastLine = line; + } + hello.release(); } } catch (IOException e) { - _log.error("Caught IOException", e); throw new LCDException(e); } _log.debug("Terminating");