[denonmarantz] Run the Telnet socket in a dedicated thread (#9511)

* Run the Telnet socket in a dedicated thread, not from the shared thread pool. Fixes #9494 Signed-off-by: Jan-Willem Veldhuis <[email protected]>
openhab · Dec 26, 2020 · d014ac6 · d014ac6
1 parent 9d36c31
commit d014ac6
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 27 deletions.
diff --git a/...ava/org/openhab/binding/denonmarantz/internal/connector/DenonMarantzConnectorFactory.java b/...ava/org/openhab/binding/denonmarantz/internal/connector/DenonMarantzConnectorFactory.java
@@ -29,9 +29,9 @@
 public class DenonMarantzConnectorFactory {
 
     public DenonMarantzConnector getConnector(DenonMarantzConfiguration config, DenonMarantzState state,
-            ScheduledExecutorService scheduler, HttpClient httpClient) {
+            ScheduledExecutorService scheduler, HttpClient httpClient, String thingUID) {
         if (config.isTelnet()) {
-            return new DenonMarantzTelnetConnector(config, state, scheduler);
+            return new DenonMarantzTelnetConnector(config, state, scheduler, thingUID);
         } else {
             return new DenonMarantzHttpConnector(config, state, scheduler, httpClient);
         }

diff --git a/...ctor/telnet/DenonMarantzTelnetClient.java → ...elnet/DenonMarantzTelnetClientThread.java b/...ctor/telnet/DenonMarantzTelnetClient.java → ...elnet/DenonMarantzTelnetClientThread.java
@@ -31,9 +31,9 @@
  * @author Jeroen Idserda - Initial contribution (1.x Binding)
  * @author Jan-Willem Veldhuis - Refactored for 2.x
  */
-public class DenonMarantzTelnetClient implements Runnable {
+public class DenonMarantzTelnetClientThread extends Thread {
 
-    private Logger logger = LoggerFactory.getLogger(DenonMarantzTelnetClient.class);
+    private Logger logger = LoggerFactory.getLogger(DenonMarantzTelnetClientThread.class);
 
     private static final Integer RECONNECT_DELAY = 60000; // 1 minute
 
@@ -43,8 +43,6 @@ public class DenonMarantzTelnetClient implements Runnable {
 
     private DenonMarantzTelnetListener listener;
 
-    private boolean running = true;
-
     private boolean connected = false;
 
     private Socket socket;
@@ -53,15 +51,15 @@ public class DenonMarantzTelnetClient implements Runnable {
 
     private BufferedReader in;
 
-    public DenonMarantzTelnetClient(DenonMarantzConfiguration config, DenonMarantzTelnetListener listener) {
+    public DenonMarantzTelnetClientThread(DenonMarantzConfiguration config, DenonMarantzTelnetListener listener) {
         logger.debug("Denon listener created");
         this.config = config;
         this.listener = listener;
     }
 
     @Override
     public void run() {
-        while (running) {
+        while (!isInterrupted()) {
             if (!connected) {
                 connectTelnetSocket();
             }
@@ -90,20 +88,21 @@ public void run() {
                         connected = false;
                     }
                 } catch (IOException e) {
-                    if (!Thread.currentThread().isInterrupted()) {
+                    if (!isInterrupted()) {
                         // only log if we don't stop this on purpose causing a SocketClosed
                         logger.debug("Error in telnet connection ", e);
                     }
                     connected = false;
                     listener.telnetClientConnected(false);
                 }
-            } while (running && connected);
+            } while (!isInterrupted() && connected);
         }
+        disconnect();
+        logger.debug("Stopped client thread");
     }
 
     public void sendCommand(String command) {
         if (out != null) {
-            logger.debug("Sending command {}", command);
             try {
                 out.write(command + '\r');
                 out.flush();
@@ -116,21 +115,21 @@ public void sendCommand(String command) {
     }
 
     public void shutdown() {
-        this.running = false;
         disconnect();
     }
 
     private void connectTelnetSocket() {
         disconnect();
         int delay = 0;
 
-        while (this.running && (socket == null || !socket.isConnected())) {
+        while (!isInterrupted() && (socket == null || !socket.isConnected())) {
             try {
                 Thread.sleep(delay);
                 logger.debug("Connecting to {}", config.getHost());
 
-                // Use raw socket instead of TelnetClient here because TelnetClient sends an extra newline char
-                // after each write which causes the connection to become unresponsive.
+                // Use raw socket instead of TelnetClient here because TelnetClient sends an
+                // extra newline char after each write which causes the connection to become
+                // unresponsive.
                 socket = new Socket();
                 socket.connect(new InetSocketAddress(config.getHost(), config.getTelnetPort()), TIMEOUT);
                 socket.setKeepAlive(true);
@@ -141,6 +140,7 @@ private void connectTelnetSocket() {
 
                 connected = true;
                 listener.telnetClientConnected(true);
+                logger.debug("Denon telnet client connected to {}", config.getHost());
             } catch (IOException e) {
                 logger.debug("Cannot connect to {}", config.getHost(), e);
                 listener.telnetClientConnected(false);
@@ -149,8 +149,6 @@ private void connectTelnetSocket() {
             }
             delay = RECONNECT_DELAY;
         }
-
-        logger.debug("Denon telnet client connected to {}", config.getHost());
     }
 
     public boolean isConnected() {

diff --git a/...g/openhab/binding/denonmarantz/internal/connector/telnet/DenonMarantzTelnetConnector.java b/...g/openhab/binding/denonmarantz/internal/connector/telnet/DenonMarantzTelnetConnector.java
@@ -46,30 +46,32 @@ public class DenonMarantzTelnetConnector extends DenonMarantzConnector implement
 
     private static final BigDecimal NINETYNINE = new BigDecimal("99");
 
-    private DenonMarantzTelnetClient telnetClient;
+    private DenonMarantzTelnetClientThread telnetClientThread;
 
     private boolean displayNowplaying = false;
 
     protected boolean disposing = false;
 
     private Future<?> telnetStateRequest;
 
-    private Future<?> telnetRunnable;
+    private String thingUID;
 
     public DenonMarantzTelnetConnector(DenonMarantzConfiguration config, DenonMarantzState state,
-            ScheduledExecutorService scheduler) {
+            ScheduledExecutorService scheduler, String thingUID) {
         this.config = config;
         this.scheduler = scheduler;
         this.state = state;
+        this.thingUID = thingUID;
     }
 
     /**
      * Set up the connection to the receiver. Either using Telnet or by polling the HTTP API.
      */
     @Override
     public void connect() {
-        telnetClient = new DenonMarantzTelnetClient(config, this);
-        telnetRunnable = scheduler.submit(telnetClient);
+        telnetClientThread = new DenonMarantzTelnetClientThread(config, this);
+        telnetClientThread.setName("OH-binding-" + thingUID);
+        telnetClientThread.start();
     }
 
     @Override
@@ -98,9 +100,12 @@ public void dispose() {
             telnetStateRequest = null;
         }
 
-        if (telnetClient != null && !telnetRunnable.isDone()) {
-            telnetRunnable.cancel(true);
-            telnetClient.shutdown();
+        if (telnetClientThread != null) {
+            telnetClientThread.interrupt();
+            // Invoke a shutdown after interrupting the thread to close the socket immediately,
+            // otherwise the client keeps running until a line was received from the telnet connection
+            telnetClientThread.shutdown();
+            telnetClientThread = null;
         }
     }
 
@@ -262,7 +267,7 @@ protected void internalSendCommand(String command) {
             logger.warn("Trying to send empty command");
             return;
         }
-        telnetClient.sendCommand(command);
+        telnetClientThread.sendCommand(command);
     }
 
     /**

diff --git a/.../src/main/java/org/openhab/binding/denonmarantz/internal/handler/DenonMarantzHandler.java b/.../src/main/java/org/openhab/binding/denonmarantz/internal/handler/DenonMarantzHandler.java
@@ -314,7 +314,8 @@ private void createConnection() {
         if (connector != null) {
             connector.dispose();
         }
-        connector = connectorFactory.getConnector(config, denonMarantzState, scheduler, httpClient);
+        connector = connectorFactory.getConnector(config, denonMarantzState, scheduler, httpClient,
+                this.getThing().getUID().getAsString());
         connector.connect();
     }