From 24a01e44efaa8e1ebd3f3546b7c719d943ba9f9c Mon Sep 17 00:00:00 2001 From: alexf2015 Date: Mon, 31 Aug 2020 19:18:37 +0200 Subject: [PATCH] [Digitalstrom] Bugfixes/Improvements (#8372) * added specific error handling for SSLHandshakeExecptions logging * Scene calls sometimes get lost, thus the scene state is invalid and gets stuck. Therefore scene activation events should always trigger the activation logic, even if the internal state is already "active" * fixed a "loop" in restart process which resulted in a stack overflow exception and could even affect whole openhab instance. * this should fix #8214 which is caused by a multi-threaded access to "pollingSchedulers" Signed-off-by: Alexander Friese Signed-off-by: Daan Meijer --- .../internal/handler/BridgeHandler.java | 13 ++++++++-- .../lib/listener/ConnectionListener.java | 4 ++++ .../lib/manager/ConnectionManager.java | 1 + .../manager/impl/ConnectionManagerImpl.java | 18 ++++++-------- .../lib/manager/impl/SceneManagerImpl.java | 4 ++++ .../AbstractSensorJobExecutor.java | 14 ++++++++--- .../impl/HttpTransportImpl.java | 7 +++--- .../lib/structure/scene/InternalScene.java | 24 ++++++++++++------- 8 files changed, 58 insertions(+), 27 deletions(-) diff --git a/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/handler/BridgeHandler.java b/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/handler/BridgeHandler.java index 40e308dc3d1b1..cacd906e2c5f3 100644 --- a/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/handler/BridgeHandler.java +++ b/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/handler/BridgeHandler.java @@ -549,13 +549,13 @@ public void onConnectionStateChange(String newConnectionState) { return; case CONNECTION_RESUMED: if (connectionTimeoutCounter > 0) { + // reset connection timeout counter + connectionTimeoutCounter = 0; if (connMan.checkConnection()) { restartServices(); setStatus(ThingStatus.ONLINE); } } - // reset connection timeout counter - connectionTimeoutCounter = 0; return; case APPLICATION_TOKEN_GENERATED: if (connMan != null) { @@ -668,7 +668,16 @@ public void onConnectionStateChange(String newConnectionState, String reason) { case INVALID_URL: updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, "Invalid URL is set."); break; + case CONNECTION_LOST: + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, + "IOException / Connection lost."); + break; + case SSL_HANDSHAKE_ERROR: + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, + "SSL Handshake error / Connection lost."); + break; default: + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, reason); } // reset connection timeout counter connectionTimeoutCounter = 0; diff --git a/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/listener/ConnectionListener.java b/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/listener/ConnectionListener.java index d6f8bffccf6a0..8dfe4cb5229c5 100644 --- a/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/listener/ConnectionListener.java +++ b/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/listener/ConnectionListener.java @@ -30,6 +30,10 @@ public interface ConnectionListener { * State, if the connection to the digitalSTROM-Server is lost. */ final String CONNECTION_LOST = "connLost"; + /** + * State, if a ssl handshake problem occured while communicating with the digitalSTROM-Server. + */ + final String SSL_HANDSHAKE_ERROR = "sslHandshakeError"; /** * State, if the connection to the digitalSTROM-Server is resumed. */ diff --git a/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/manager/ConnectionManager.java b/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/manager/ConnectionManager.java index 240e8cebf9081..896a026408e45 100644 --- a/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/manager/ConnectionManager.java +++ b/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/manager/ConnectionManager.java @@ -31,6 +31,7 @@ public interface ConnectionManager { public static final int SOCKET_TIMEOUT_EXCEPTION = -4; public static final int UNKNOWN_HOST_EXCEPTION = -5; public static final int AUTHENTIFICATION_PROBLEM = -6; + public static final int SSL_HANDSHAKE_EXCEPTION = -7; /** * Returns the {@link HttpTransport} to execute queries or special commands on the digitalSTROM-Server. diff --git a/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/manager/impl/ConnectionManagerImpl.java b/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/manager/impl/ConnectionManagerImpl.java index f0b82b081adb4..ad397c564696c 100644 --- a/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/manager/impl/ConnectionManagerImpl.java +++ b/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/manager/impl/ConnectionManagerImpl.java @@ -314,7 +314,6 @@ public boolean checkConnection(int code) { case HttpURLConnection.HTTP_INTERNAL_ERROR: case HttpURLConnection.HTTP_OK: if (!connectionEstablished) { - connectionEstablished = true; onConnectionResumed(); } break; @@ -326,7 +325,6 @@ public boolean checkConnection(int code) { if (sessionToken != null) { if (!connectionEstablished) { onConnectionResumed(); - connectionEstablished = true; } } else { if (this.genAppToken) { @@ -337,22 +335,19 @@ public boolean checkConnection(int code) { break; case ConnectionManager.MALFORMED_URL_EXCEPTION: onConnectionLost(ConnectionListener.INVALID_URL); - connectionEstablished = false; break; case ConnectionManager.CONNECTION_EXCEPTION: case ConnectionManager.SOCKET_TIMEOUT_EXCEPTION: onConnectionLost(ConnectionListener.CONNECTON_TIMEOUT); - connectionEstablished = false; + break; + case ConnectionManager.SSL_HANDSHAKE_EXCEPTION: + onConnectionLost(ConnectionListener.SSL_HANDSHAKE_ERROR); break; case ConnectionManager.GENERAL_EXCEPTION: - if (connListener != null) { - connListener.onConnectionStateChange(ConnectionListener.CONNECTION_LOST); - } + onConnectionLost(ConnectionListener.CONNECTION_LOST); break; case ConnectionManager.UNKNOWN_HOST_EXCEPTION: - if (connListener != null) { - onConnectionLost(ConnectionListener.UNKNOWN_HOST); - } + onConnectionLost(ConnectionListener.UNKNOWN_HOST); break; case ConnectionManager.AUTHENTIFICATION_PROBLEM: if (connListener != null) { @@ -367,7 +362,6 @@ public boolean checkConnection(int code) { break; case HttpURLConnection.HTTP_NOT_FOUND: onConnectionLost(ConnectionListener.HOST_NOT_FOUND); - connectionEstablished = false; break; } return connectionEstablished; @@ -485,6 +479,7 @@ private void onConnectionLost(String reason) { if (connListener != null) { connListener.onConnectionStateChange(ConnectionListener.CONNECTION_LOST, reason); } + connectionEstablished = false; } /** @@ -494,6 +489,7 @@ private void onConnectionResumed() { if (connListener != null) { connListener.onConnectionStateChange(ConnectionListener.CONNECTION_RESUMED); } + connectionEstablished = true; } @Override diff --git a/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/manager/impl/SceneManagerImpl.java b/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/manager/impl/SceneManagerImpl.java index 247eeee541644..ad238eb207e06 100644 --- a/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/manager/impl/SceneManagerImpl.java +++ b/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/manager/impl/SceneManagerImpl.java @@ -262,10 +262,12 @@ public void callInternalSceneWithoutDiscovery(Integer zoneID, Short groupID, Sho public void callInternalScene(String sceneID) { InternalScene intScene = this.internalSceneMap.get(sceneID); if (intScene != null) { + logger.debug("activating existing scene {}", intScene.getSceneName()); intScene.activateScene(); } else { intScene = createNewScene(sceneID); if (intScene != null) { + logger.debug("created new scene, activating it: {}", intScene.getSceneName()); discovery.sceneDiscoverd(intScene); intScene.activateScene(); } @@ -373,10 +375,12 @@ public void undoInternalScene(InternalScene scene) { public void undoInternalScene(String sceneID) { InternalScene intScene = this.internalSceneMap.get(sceneID); if (intScene != null) { + logger.debug("deactivating existing scene {}", intScene.getSceneName()); intScene.deactivateScene(); } else { intScene = createNewScene(sceneID); if (intScene != null) { + logger.debug("created new scene, deactivating it: {}", intScene.getSceneName()); intScene.deactivateScene(); } } diff --git a/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/sensorjobexecutor/AbstractSensorJobExecutor.java b/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/sensorjobexecutor/AbstractSensorJobExecutor.java index 33fee86c8775a..a2d6848f076ef 100644 --- a/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/sensorjobexecutor/AbstractSensorJobExecutor.java +++ b/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/sensorjobexecutor/AbstractSensorJobExecutor.java @@ -68,13 +68,21 @@ public ExecutorRunnable(CircuitScheduler circuit) { @Override public void run() { + // pollingSchedulers is not final and might be set to null by another thread. See #8214 + Map> pollingSchedulers = AbstractSensorJobExecutor.this.pollingSchedulers; + SensorJob sensorJob = circuit.getNextSensorJob(); + DSID meter = circuit.getMeterDSID(); + if (sensorJob != null) { sensorJob.execute(dSAPI, connectionManager.getSessionToken()); } - if (circuit.noMoreJobs()) { - logger.debug("no more jobs... stop circuit schedduler with id = {}", circuit.getMeterDSID()); - pollingSchedulers.get(circuit.getMeterDSID()).cancel(true); + if (circuit.noMoreJobs() && pollingSchedulers != null) { + logger.debug("no more jobs... stop circuit schedduler with id = {}", meter); + ScheduledFuture scheduler = pollingSchedulers.get(meter); + if (scheduler != null) { + scheduler.cancel(true); + } } } } diff --git a/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/serverconnection/impl/HttpTransportImpl.java b/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/serverconnection/impl/HttpTransportImpl.java index 6953eb8a76ffc..77cea9a99a831 100644 --- a/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/serverconnection/impl/HttpTransportImpl.java +++ b/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/serverconnection/impl/HttpTransportImpl.java @@ -35,6 +35,7 @@ import javax.net.ssl.HostnameVerifier; import javax.net.ssl.HttpsURLConnection; import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLHandshakeException; import javax.net.ssl.SSLSession; import javax.net.ssl.SSLSocketFactory; import javax.net.ssl.TrustManager; @@ -296,11 +297,11 @@ public String execute(String request, int connectTimeout, int readTimeout) { informConnectionManager(ConnectionManager.MALFORMED_URL_EXCEPTION); } catch (java.net.UnknownHostException e) { informConnectionManager(ConnectionManager.UNKNOWN_HOST_EXCEPTION); + } catch (SSLHandshakeException e) { + informConnectionManager(ConnectionManager.SSL_HANDSHAKE_EXCEPTION); } catch (IOException e) { logger.error("An IOException occurred: ", e); - if (connectionManager != null) { - informConnectionManager(ConnectionManager.GENERAL_EXCEPTION); - } + informConnectionManager(ConnectionManager.GENERAL_EXCEPTION); } finally { if (connection != null) { connection.disconnect(); diff --git a/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/structure/scene/InternalScene.java b/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/structure/scene/InternalScene.java index 05fcc3fde827d..cdfbb43db1bc9 100644 --- a/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/structure/scene/InternalScene.java +++ b/bundles/org.openhab.binding.digitalstrom/src/main/java/org/openhab/binding/digitalstrom/internal/lib/structure/scene/InternalScene.java @@ -20,6 +20,8 @@ import org.openhab.binding.digitalstrom.internal.lib.listener.SceneStatusListener; import org.openhab.binding.digitalstrom.internal.lib.structure.devices.Device; import org.openhab.binding.digitalstrom.internal.lib.structure.scene.constants.SceneTypes; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * The {@link InternalScene} represents a digitalSTROM-Scene for the internal model. @@ -28,6 +30,7 @@ * @author Matthias Siegele - Initial contribution */ public class InternalScene { + private final Logger logger = LoggerFactory.getLogger(InternalScene.class); private final Short sceneID; private final Short groupID; @@ -91,14 +94,13 @@ private void setSceneType() { * Activates this Scene. */ public void activateScene() { - if (!active) { - this.active = true; - deviceHasChanged = false; - informListener(); - if (this.devices != null) { - for (Device device : this.devices) { - device.callInternalScene(this); - } + logger.debug("activate scene: {}", this.getSceneName()); + this.active = true; + deviceHasChanged = false; + informListener(); + if (this.devices != null) { + for (Device device : this.devices) { + device.callInternalScene(this); } } } @@ -107,6 +109,7 @@ public void activateScene() { * Deactivates this Scene. */ public void deactivateScene() { + logger.debug("deactivate scene: {}", this.getSceneName()); if (active) { this.active = false; deviceHasChanged = false; @@ -123,6 +126,7 @@ public void deactivateScene() { * Will be called by a device, if an undo call of an other scene activated this scene. */ public void activateSceneByDevice() { + logger.debug("activate scene by device: {}", this.getSceneName()); if (!active && !deviceHasChanged) { this.active = true; deviceHasChanged = false; @@ -134,6 +138,7 @@ public void activateSceneByDevice() { * Will be called by a device, if an call of an other scene deactivated this scene. */ public void deactivateSceneByDevice() { + logger.debug("deactivate scene by device: {}", this.getSceneName()); if (active) { this.active = false; deviceHasChanged = false; @@ -157,8 +162,11 @@ public void deviceSceneChanged(short sceneNumber) { } private void informListener() { + logger.debug("inform listener: {}", this.getSceneName()); if (this.listener != null) { listener.onSceneStateChanged(this.active); + } else { + logger.debug("no listener found for scene: {}", this.getSceneName()); } }