Improved the reconnect behaviour of CULNetworkHandlerImpl #4359
Conversation
|
great thanks @dereulenspiegel! Could the other CUL users please have a look at this one please? |
|
Looks quite good, small suggestion, in CULSerialHandlerImpl, can you remove/make local is und os, they are never accessed directly so therefore they aren't needed. |
… variables any more, since no one needs access to them.
|
InputStream and OutputStream aren't class variables any more. |
|
can any of the others @cyclingengineer @querdenker2k @johgoe confirm this working? |
| if (bw == null) { | ||
| log.error("Can't write message, BufferedWriter is NULL"); | ||
| } | ||
| synchronized (bw) { |
There was a problem hiding this comment.
New implementation CULSerialHandlerImpl is not synchronized. What happens in parallel write message events?
|
@teichsta, @dereulenspiegel The normal good case with a CUL is working, but I'm not sure about the parallel and error cases. See my comments in the code. |
…tion and now trying to reopen connection on connection error
|
The serial connection part wasn't original part of my changes. It was merely touched because I changed the logic in AbstractCULHandler. |
|
Is there a consensus that this is ready to be merged? ( @dereulenspiegel @tarioch @johgoe @querdenker2k @cyclingengineer ) My review of the changes shows only that the source needs to be run through the eclipse formatter... |
|
From my point of view, its ok to merge this. |
|
Good from my side as well |
| * @since 1.5.0 | ||
| */ | ||
| public class CULNetworkHandlerImpl extends AbstractCULHandler<CULNetworkConfig> { | ||
| public class CULNetworkHandlerImpl extends AbstractCULHandler<CULNetworkConfig>implements Runnable { |
There was a problem hiding this comment.
a lack of space before implements suggests the Eclipse IDE formatting wasn't applied or the settings are incorrect.
|
|
||
| try { | ||
| synchronized (bw) { | ||
| if (bw == null) { |
There was a problem hiding this comment.
You can't synchronize on a null object. If there is a chance that bw is null, then this code should be changed (and anywhere else where X can be null and there is a synchronized (X) construct).
There was a problem hiding this comment.
@watou, would you be ok with merging this PR as is if I submit a separate PR to address the formatting and null synchronization issues?
There was a problem hiding this comment.
@9037568 fine with me. Should this and your follow-on PR target 1.9.1 or is that too risky?
There was a problem hiding this comment.
Probably enough change there to not call it a patch release. Let's call it 1.10.0.
|
I had a chance to test the pull request against my setup (cul connected to ser2net). It seems to work fine and recovers great from ser2net restarts |
Enhancement for openhab#4359.
* Apply Eclipse formatting. Enhancement for #4359. * Various cleanup of log messages, including string concatenation and log levels. * Fix synchronization issues. Shouldn't be synchronizing on objects that are about to be modified.
This Pull Request addresses the issues described in #4035. I replaced the current implementation of CULNetworkHandlerImpl with a nio based one to simplify handling of connection errors.
To enable this change slight changes in other components of the CUL transport were necessary. Mainly the responsibility of low level communication is now in the respective handler implementations and not in the AbstractCULHandler any more.