Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[innogysmarthome] Reconnect fixes (#8182) #8353

Merged
merged 38 commits into from
Sep 2, 2020
Merged

[innogysmarthome] Reconnect fixes (#8182) #8353

merged 38 commits into from
Sep 2, 2020

Conversation

Novanic
Copy link
Contributor

@Novanic Novanic commented Aug 27, 2020

This pull-request fixes a few reconnect problems regarding the WebSocket, see issue #8182 for more information

  • When an error occurred while starting the websocket, no reconnect attempt was started which caused that the binding loses the connection (till the next restart of the server or binding), see issue [innogysmarthome] Insufficient reconnect attempts #8182 for more information
  • When an error occurs, it is now waited at least for 30 seconds for the next reconnect attempt to avoid too many requests (on re-occurring errors).
  • Fix for a side-effect of "Fix possible resource leak [innogysmarthome] Fix possible resource leak #8080" (the websocket were closed unnormally by default which could trigger false and about one reconnect attempt per second).
  • NullPointerException fixed
  • An Exception could cancel the scheduler, so no reconnect attempt was executed.
  • An Exception could get the binding in a mode which executed unnecessary reconnects at a fixed rate.
  • First automated tests added for the binding.

Novanic and others added 30 commits June 10, 2020 19:50
…ore it wasn't rescheduled, because startClient is called by a scheduler which is everytime still running, so the detection was broken)

Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]>
… when an error occurs it is scheduled automatically again (so it is useless and disturbing the it is re-scheduled automatically with a fixed rate).

Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]>
@Novanic Novanic requested a review from ollie-dev as a code owner August 27, 2020 09:11
…Exception(...), the scheduler was canceled again, because the code ran normally again after the Exception handling which reaches cancelReinitJob()... That caused that the scheduler wasn't executed in this case...

Signed-off-by: Sven Strohschein <[email protected]>
@TravisBuddy
Copy link

Travis tests were successful

Hey @Novanic,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@Hilbrand Hilbrand added the bug An unexpected problem or unintended behavior of an add-on label Aug 27, 2020
@openhab openhab deleted a comment from TravisBuddy Aug 29, 2020
Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'll keep it open for a few more days so you can test if it's running ok this time 😉

@Hilbrand Hilbrand added this to the 2.5.9 milestone Aug 29, 2020
@Novanic
Copy link
Contributor Author

Novanic commented Aug 29, 2020

Ok. :-)

Here is a built version with the changes for testing, some people of a OpenHAB Innogy Facebook group seem also to test it now: https://www.dropbox.com/s/5vv5qpnznelq10d/org.openhab.binding.innogysmarthome-2.5.9-SNAPSHOT.jar?dl=0

@Hilbrand
Copy link
Member

Hilbrand commented Sep 1, 2020

Looks like you need to fix the conflicts cause by your other pr: #8375

@TravisBuddy
Copy link

Travis tests were successful

Hey @Novanic,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@Novanic
Copy link
Contributor Author

Novanic commented Sep 2, 2020

I hate Git... Just forgot to add Signed-off-by to the commit message.............. I think I have to create a new branch and a new pull-request........

@TravisBuddy
Copy link

Travis tests were successful

Hey @Novanic,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Signed-off-by: Sven Strohschein <[email protected]>
@Hilbrand
Copy link
Member

Hilbrand commented Sep 2, 2020

Looks like everything is ok? No need to create a new pr. When having to fix previous commits you can always use git rebase -i HEAD~<number of commits to fix>. and than push force the changes back.

@TravisBuddy
Copy link

Travis tests were successful

Hey @Novanic,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@Novanic
Copy link
Contributor Author

Novanic commented Sep 2, 2020

Yes, I had luck that there was only the last commit message without sign-off. I had problems in the past with rebase and merge (rebases and merge commits weren't recognized correctly, all commits of other people were added to the pull-request). I think the problem is that I'm working with a fork and creating pull-requests from a branch of the fork... I should give up the fork for the next pull-requests.

@Novanic
Copy link
Contributor Author

Novanic commented Sep 2, 2020

I think the pull-request could now get merged. At least 3 people tested the version for a few days and there were some reconnect attempts which were all successfully. The greatest risk is that I did something wrong on fixing the branch today... ;-)

@Hilbrand Hilbrand merged commit aa32b69 into openhab:2.5.x Sep 2, 2020
CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Sep 12, 2020
* startClient can now also get rescheduled when the method crashes (before it wasn't rescheduled, because startClient is called by a scheduler which is everytime still running, so the detection was broken)
* The re-init job isn't scheduled for more than 1 time anymore, because when an error occurs it is scheduled automatically again (so it is useless and disturbing the it is re-scheduled automatically with a fixed rate).
* Tests for InnogyBridgeHandler added
* Bug-Fix: When an Exception occurred which was handled by handleClientException(...), the scheduler was canceled again, because the code ran normally again after the Exception handling which reaches cancelReinitJob()... That caused that the scheduler wasn't executed in this case...
* Warning message improved

Closes openhab#8182

Signed-off-by: Sven Strohschein <[email protected]>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
* startClient can now also get rescheduled when the method crashes (before it wasn't rescheduled, because startClient is called by a scheduler which is everytime still running, so the detection was broken)
* The re-init job isn't scheduled for more than 1 time anymore, because when an error occurs it is scheduled automatically again (so it is useless and disturbing the it is re-scheduled automatically with a fixed rate).
* Tests for InnogyBridgeHandler added
* Bug-Fix: When an Exception occurred which was handled by handleClientException(...), the scheduler was canceled again, because the code ran normally again after the Exception handling which reaches cancelReinitJob()... That caused that the scheduler wasn't executed in this case...
* Warning message improved

Closes openhab#8182

Signed-off-by: Sven Strohschein <[email protected]>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Oct 8, 2020
* startClient can now also get rescheduled when the method crashes (before it wasn't rescheduled, because startClient is called by a scheduler which is everytime still running, so the detection was broken)
* The re-init job isn't scheduled for more than 1 time anymore, because when an error occurs it is scheduled automatically again (so it is useless and disturbing the it is re-scheduled automatically with a fixed rate).
* Tests for InnogyBridgeHandler added
* Bug-Fix: When an Exception occurred which was handled by handleClientException(...), the scheduler was canceled again, because the code ran normally again after the Exception handling which reaches cancelReinitJob()... That caused that the scheduler wasn't executed in this case...
* Warning message improved

Closes openhab#8182

Signed-off-by: Sven Strohschein <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants