Added reconnection capabilities for the test client. #216
Conversation
| LOG.debug("Attempted to access a connection-dependent object while the connection is interrupted, " | ||
| + "wait for connection to be re-established."); | ||
| try { | ||
| lock.wait(); |
There was a problem hiding this comment.
The calling thread does not always get awakened, especially when this wait is called after the notifyAll, but before inReconnectProcess gets set back to false, which is possible, because inReconnectProcess gets set outside of the respective synchronized block.
There was a problem hiding this comment.
inReconnectProcess is set in the synchronized block now
| @@ -384,12 +435,12 @@ public SdcRemoteDevicesConnector getConnector() { | |||
|
|
|||
| @Override | |||
| public SdcRemoteDevice getSdcRemoteDevice() { | |||
| return sdcRemoteDevice; | |||
| return restrictedGetter(sdcRemoteDevice); | |||
There was a problem hiding this comment.
This will return the old object after a wait and not the new one, which matters alot because the reconnect potentially replaces its value.
There was a problem hiding this comment.
Changed the restrictedGetter implementation, the new object should be returned now
| this.roomSearchLogString, | ||
| this.bedSearchLogString, | ||
| e); | ||
| if (inReconnectProcess.get()) { |
There was a problem hiding this comment.
To make the concept work, inReconnectProcess would probably have to be used in many more cases and functions similiarly to how it is used here and also it would have to be thread id aware.
For example I got a TimeoutException in https://github.com/Draegerwerk/SDCcc/pull/216/files#diff-955d82d123e93ce4675e6524a4b941f007756b1589cfb87114b82fffa9e2d181R380 during a reconnect and although the reconnect was successful on the second attempt, the test run still got invalidated.
There was a problem hiding this comment.
Added a whitelist to the log appender to not invalid the test run during reconnect.
…ocess" inside synchronize block
…tablish-data-exchange
| } | ||
|
|
||
| @Override | ||
| public void shouldReconnect(final Boolean shouldReconnect) { |
There was a problem hiding this comment.
Calling this with false as a parameter may lead to log messages from before to not be affected by the respective whitelist as they should, because of them potentially being processed in the appender, which is using the whitelist being set, too late e.g. after this returns.
| } | ||
|
|
||
| @Override | ||
| public void shouldReconnect(final Boolean shouldReconnect) { |
There was a problem hiding this comment.
It is unclear when to call this with false as its parameter value, I think it should be restricted to when all of the following are true:
- all log messages of whitelisted threads have been fully processed
- the maximum amount of retries occurred without a successful one, or, there was a successful try and no further ones will occur
- the expected behaviour e.g. a disconnect was observed at least once
Perhaps this should be checked here or else some convenience function provided to wait for these events before calling this function.
| } else { | ||
| LOG.info("Watchdog detected expected disconnect from provider."); | ||
| LOG.info("The disconnect from provider was expected."); |
There was a problem hiding this comment.
| LOG.info("The disconnect from provider was expected."); | |
| LOG.info("The disconnect from the provider was expected."); |
Reconnection attempts on connection loss can be enabled in the test client.
Checklist
The following aspects have been respected by the author of this pull request, confirmed by both pull request assignee and reviewer: