Skip to content

Commit

Permalink
[Digitalstrom] Bugfixes/Improvements (#8372)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>
  • Loading branch information
@alexf2015
alexf2015 authored Aug 31, 2020
1 parent 981698a commit 124d9d9
Show file tree
Hide file tree
Showing 8 changed files with 58 additions and 27 deletions.
13 changes: 11 additions & 2 deletions ...lstrom/src/main/java/org/openhab/binding/digitalstrom/internal/handler/BridgeHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down
4 changes: 4 additions & 0 deletions .../main/java/org/openhab/binding/digitalstrom/internal/lib/listener/ConnectionListener.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down
1 change: 1 addition & 0 deletions ...rc/main/java/org/openhab/binding/digitalstrom/internal/lib/manager/ConnectionManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
18 changes: 7 additions & 11 deletions ...ava/org/openhab/binding/digitalstrom/internal/lib/manager/impl/ConnectionManagerImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,6 @@ public boolean checkConnection(int code) {
case HttpURLConnection.HTTP_INTERNAL_ERROR:
case HttpURLConnection.HTTP_OK:
if (!connectionEstablished) {
connectionEstablished = true;
onConnectionResumed();
}
break;
Expand All @@ -326,7 +325,6 @@ public boolean checkConnection(int code) {
if (sessionToken != null) {
if (!connectionEstablished) {
onConnectionResumed();
connectionEstablished = true;
}
} else {
if (this.genAppToken) {
Expand All @@ -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) {
Expand All @@ -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;
Expand Down Expand Up @@ -485,6 +479,7 @@ private void onConnectionLost(String reason) {
if (connListener != null) {
connListener.onConnectionStateChange(ConnectionListener.CONNECTION_LOST, reason);
}
connectionEstablished = false;
}

/**
Expand All @@ -494,6 +489,7 @@ private void onConnectionResumed() {
if (connListener != null) {
connListener.onConnectionStateChange(ConnectionListener.CONNECTION_RESUMED);
}
connectionEstablished = true;
}

@Override
Expand Down
4 changes: 4 additions & 0 deletions ...ain/java/org/openhab/binding/digitalstrom/internal/lib/manager/impl/SceneManagerImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down Expand Up @@ -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();
}
}
Expand Down
14 changes: 11 additions & 3 deletions ...penhab/binding/digitalstrom/internal/lib/sensorjobexecutor/AbstractSensorJobExecutor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
}
Expand Down
7 changes: 4 additions & 3 deletions ...rg/openhab/binding/digitalstrom/internal/lib/serverconnection/impl/HttpTransportImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down
24 changes: 16 additions & 8 deletions ...ain/java/org/openhab/binding/digitalstrom/internal/lib/structure/scene/InternalScene.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
}
}
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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());
}
}

Expand Down

0 comments on commit 124d9d9

Please sign in to comment.