From 19a9833fbd99193e484d8290d60c6cf6976fc09f Mon Sep 17 00:00:00 2001 From: jwveldhuis Date: Sat, 26 Dec 2020 20:21:21 +0100 Subject: [PATCH] [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 Signed-off-by: Joseph Hagberg --- .../DenonMarantzConnectorFactory.java | 4 +-- ...va => DenonMarantzTelnetClientThread.java} | 28 +++++++++---------- .../telnet/DenonMarantzTelnetConnector.java | 23 +++++++++------ .../internal/handler/DenonMarantzHandler.java | 3 +- 4 files changed, 31 insertions(+), 27 deletions(-) rename bundles/org.openhab.binding.denonmarantz/src/main/java/org/openhab/binding/denonmarantz/internal/connector/telnet/{DenonMarantzTelnetClient.java => DenonMarantzTelnetClientThread.java} (87%) diff --git a/bundles/org.openhab.binding.denonmarantz/src/main/java/org/openhab/binding/denonmarantz/internal/connector/DenonMarantzConnectorFactory.java b/bundles/org.openhab.binding.denonmarantz/src/main/java/org/openhab/binding/denonmarantz/internal/connector/DenonMarantzConnectorFactory.java index 50bab9745cba1..dac91cc8c650f 100644 --- a/bundles/org.openhab.binding.denonmarantz/src/main/java/org/openhab/binding/denonmarantz/internal/connector/DenonMarantzConnectorFactory.java +++ b/bundles/org.openhab.binding.denonmarantz/src/main/java/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/bundles/org.openhab.binding.denonmarantz/src/main/java/org/openhab/binding/denonmarantz/internal/connector/telnet/DenonMarantzTelnetClient.java b/bundles/org.openhab.binding.denonmarantz/src/main/java/org/openhab/binding/denonmarantz/internal/connector/telnet/DenonMarantzTelnetClientThread.java similarity index 87% rename from bundles/org.openhab.binding.denonmarantz/src/main/java/org/openhab/binding/denonmarantz/internal/connector/telnet/DenonMarantzTelnetClient.java rename to bundles/org.openhab.binding.denonmarantz/src/main/java/org/openhab/binding/denonmarantz/internal/connector/telnet/DenonMarantzTelnetClientThread.java index 425977ae2859a..809cecde708e6 100644 --- a/bundles/org.openhab.binding.denonmarantz/src/main/java/org/openhab/binding/denonmarantz/internal/connector/telnet/DenonMarantzTelnetClient.java +++ b/bundles/org.openhab.binding.denonmarantz/src/main/java/org/openhab/binding/denonmarantz/internal/connector/telnet/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,7 +51,7 @@ 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; @@ -61,7 +59,7 @@ public DenonMarantzTelnetClient(DenonMarantzConfiguration config, DenonMarantzTe @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,7 +115,6 @@ public void sendCommand(String command) { } public void shutdown() { - this.running = false; disconnect(); } @@ -124,13 +122,14 @@ 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/bundles/org.openhab.binding.denonmarantz/src/main/java/org/openhab/binding/denonmarantz/internal/connector/telnet/DenonMarantzTelnetConnector.java b/bundles/org.openhab.binding.denonmarantz/src/main/java/org/openhab/binding/denonmarantz/internal/connector/telnet/DenonMarantzTelnetConnector.java index b5ae903f13639..f3c200272d188 100644 --- a/bundles/org.openhab.binding.denonmarantz/src/main/java/org/openhab/binding/denonmarantz/internal/connector/telnet/DenonMarantzTelnetConnector.java +++ b/bundles/org.openhab.binding.denonmarantz/src/main/java/org/openhab/binding/denonmarantz/internal/connector/telnet/DenonMarantzTelnetConnector.java @@ -46,7 +46,7 @@ public class DenonMarantzTelnetConnector extends DenonMarantzConnector implement private static final BigDecimal NINETYNINE = new BigDecimal("99"); - private DenonMarantzTelnetClient telnetClient; + private DenonMarantzTelnetClientThread telnetClientThread; private boolean displayNowplaying = false; @@ -54,13 +54,14 @@ public class DenonMarantzTelnetConnector extends DenonMarantzConnector implement 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; } /** @@ -68,8 +69,9 @@ public DenonMarantzTelnetConnector(DenonMarantzConfiguration config, DenonMarant */ @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/bundles/org.openhab.binding.denonmarantz/src/main/java/org/openhab/binding/denonmarantz/internal/handler/DenonMarantzHandler.java b/bundles/org.openhab.binding.denonmarantz/src/main/java/org/openhab/binding/denonmarantz/internal/handler/DenonMarantzHandler.java index 645ce108b05cc..bec113b839237 100644 --- a/bundles/org.openhab.binding.denonmarantz/src/main/java/org/openhab/binding/denonmarantz/internal/handler/DenonMarantzHandler.java +++ b/bundles/org.openhab.binding.denonmarantz/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(); }