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.

Sign up for GitHub

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

Jump to bottom

[lgwebos] Improved WebSocket reconnect/disconnect #8038

Merged
merged 4 commits into from
Jul 16, 2020
Merged

[lgwebos] Improved WebSocket reconnect/disconnect #8038

merged 4 commits into from
Jul 16, 2020

Conversation

lolodomo
Copy link
Contributor

Related to #8027

Signed-off-by: Laurent Garnier [email protected]

@lolodomo lolodomo requested a review from sprehn as a code owner June 29, 2020 05:55
@lolodomo lolodomo mentioned this pull request Jun 29, 2020
Closed
12 tasks
@TravisBuddy
Copy link

Travis tests have failed

Hey @lolodomo,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

sprehn
sprehn requested changes Jun 30, 2020
View reviewed changes
Copy link
Contributor

@sprehn sprehn left a comment

Choose a reason for hiding this comment

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

I am not convinced that there is a memory leak, and that it gets fixed by cancelling the future. However this may prevent some issues when establishing the connection takes longer than our polling interval.

Please do not add a condition on stopping the client.

...inding.lgwebos/src/main/java/org/openhab/binding/lgwebos/internal/LGWebOSHandlerFactory.java Outdated Show resolved Hide resolved
...ding.lgwebos/src/main/java/org/openhab/binding/lgwebos/internal/handler/LGWebOSTVSocket.java Outdated Show resolved Hide resolved
@lolodomo
Copy link
Contributor Author

lolodomo commented Jun 30, 2020 •
edited
Loading

Just to be clear, there was no memory leak in the binding, it was explained why in the issue. Or maybe only in a very particular case while the thing handler was disposed during the WebSocket connection.

@J-N-K : regarding the test with isStarted() before stopping the client, can you please comment ? You have now applied this pattern in all bindings. Should we revert ?

@lolodomo
Review comments considered
f31b4f7 
Signed-off-by: Laurent Garnier <[email protected]>
@TravisBuddy
Copy link

Hey @lolodomo,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 14a5afc0-be08-11ea-a595-8f8781864445

@lolodomo
Copy link
Contributor Author

lolodomo commented Jul 5, 2020

@sprehn : please have a new look, I added the call to destroy()

@TravisBuddy
Copy link

Travis tests have failed

Hey @lolodomo,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

@J-N-K
Copy link
Member

J-N-K commented Jul 5, 2020

Travis fails unrelated

@TravisBuddy
Copy link

Travis tests have failed

Hey @lolodomo,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

@lolodomo
Copy link
Contributor Author

Can this PR be finally merged, please ?

@kaikreuzer kaikreuzer merged commit a2ebab6 into openhab:2.5.x Jul 16, 2020
@lolodomo lolodomo deleted the lgwebos_websocket branch July 16, 2020 18:40
CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Jul 26, 2020
@lolodomo @CSchlipp
[lgwebos] Improved WebSocket reconnect/disconnect (openhab#8038)
147bae8 
Related to openhab#8027

Signed-off-by: Laurent Garnier <[email protected]>
Signed-off-by: CSchlipp <[email protected]>
MPH80 pushed a commit to MPH80/openhab-addons that referenced this pull request Aug 3, 2020
@lolodomo @MPH80
[lgwebos] Improved WebSocket reconnect/disconnect (openhab#8038)
22a00d4 
Related to openhab#8027

Signed-off-by: Laurent Garnier <[email protected]>
Signed-off-by: MPH80 <[email protected]>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@lolodomo @andrewfg
[lgwebos] Improved WebSocket reconnect/disconnect (openhab#8038)
c14db8a 
Related to openhab#8027

Signed-off-by: Laurent Garnier <[email protected]>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@lolodomo @andrewfg
[lgwebos] Improved WebSocket reconnect/disconnect (openhab#8038)
3cc2ad5 
Related to openhab#8027

Signed-off-by: Laurent Garnier <[email protected]>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@lolodomo @andrewfg
[lgwebos] Improved WebSocket reconnect/disconnect (openhab#8038)
e301cc3 
Related to openhab#8027

Signed-off-by: Laurent Garnier <[email protected]>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@lolodomo @andrewfg
[lgwebos] Improved WebSocket reconnect/disconnect (openhab#8038)
fa75abe 
Related to openhab#8027

Signed-off-by: Laurent Garnier <[email protected]>
DaanMeijer pushed a commit to DaanMeijer/openhab-addons that referenced this pull request Sep 1, 2020
@lolodomo @DaanMeijer
[lgwebos] Improved WebSocket reconnect/disconnect (openhab#8038)
f24954e 
Related to openhab#8027

Signed-off-by: Laurent Garnier <[email protected]>
Signed-off-by: Daan Meijer <[email protected]>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
@lolodomo @markus7017
[lgwebos] Improved WebSocket reconnect/disconnect (openhab#8038)
69513f3 
Related to openhab#8027

Signed-off-by: Laurent Garnier <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sprehn sprehn sprehn approved these changes

@J-N-K J-N-K J-N-K approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lolodomo @TravisBuddy @J-N-K @sprehn @kaikreuzer