[Digitalstrom] Bugfixes/Improvements (openhab#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 openhab#8214 which is caused by a multi-threaded access to "pollingSchedulers" Signed-off-by: Alexander Friese <[email protected]> Signed-off-by: Daan Meijer <[email protected]>
DaanMeijer · Sep 1, 2020 · 24a01e4 · 24a01e4
1 parent dc3452f
commit 24a01e4
Show file tree

Hide file tree

Showing 8 changed files with 58 additions and 27 deletions.
diff --git a/...lstrom/src/main/java/org/openhab/binding/digitalstrom/internal/handler/BridgeHandler.java b/...lstrom/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/.../main/java/org/openhab/binding/digitalstrom/internal/lib/listener/ConnectionListener.java b/.../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/...rc/main/java/org/openhab/binding/digitalstrom/internal/lib/manager/ConnectionManager.java b/...rc/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/...ava/org/openhab/binding/digitalstrom/internal/lib/manager/impl/ConnectionManagerImpl.java b/...ava/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/...ain/java/org/openhab/binding/digitalstrom/internal/lib/manager/impl/SceneManagerImpl.java b/...ain/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/...penhab/binding/digitalstrom/internal/lib/sensorjobexecutor/AbstractSensorJobExecutor.java b/...penhab/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<DSID, ScheduledFuture<?>> 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/...rg/openhab/binding/digitalstrom/internal/lib/serverconnection/impl/HttpTransportImpl.java b/...rg/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/...ain/java/org/openhab/binding/digitalstrom/internal/lib/structure/scene/InternalScene.java b/...ain/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());
         }
     }