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

Fix dangling SSE subscriptions #2983

Merged
merged 2 commits into from
Jun 16, 2022
Merged

Fix dangling SSE subscriptions #2983

merged 2 commits into from
Jun 16, 2022

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented May 28, 2022

Fixes #598

Unlike the sitemap SSE subscriptions generic event subscriptions did not implement a connection lost monitor.

@spacemanspiff2007 Can you confirm that this additional event does not cause issues in your application?

Signed-off-by: Jan N. Klug github@klug.nrw

@J-N-K
Fix dangling SSE subscriptions
a569f1b 
Unlike the sitemap SSE subscriptions generic event subscriptions did not implement a connection lost monitor.

Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K J-N-K added bug An unexpected problem or unintended behavior of the Core REST/SSE labels May 28, 2022
@J-N-K J-N-K requested a review from a team as a code owner May 28, 2022 12:26
@lolodomo
Copy link
Contributor

lolodomo commented May 29, 2022 •
edited
Loading

Is it for sitemap SSE?
Because I have the strange feeling I already implemented this ALIVE message several years ago...

Edit: undersood, it is not for sitemap SSE.

@spacemanspiff2007
Copy link
Contributor

@J-N-K
Thanks for asking!

Any event that can't be mapped to the corresponding python type will generate an error in the logs (every time it is received).
Under which topic is the published? Or does this not contain the topic information at all and this will only be a "ALIVE" string?

@lolodomo
Copy link
Contributor

I think about MainUI and the remote openHAB binding as other users of the OH SSE. To be checked if they will not be impacted.

@J-N-K
Copy link
Member Author

J-N-K commented May 29, 2022

@lolodomo Can you check for the remoteopenhab-binding? I have not seen issues in Main UI, but I‘ll check again.

@lolodomo
Copy link
Contributor

I have the same question as @spacemanspiff2007 : under which topic is the published?

@lolodomo
Copy link
Contributor

Hre are the topics "listened" by the remote openHAB binding: openhab/items//,openhab/things//,openhab/channels//triggered,openhab/channels//descriptionchanged

@J-N-K
Copy link
Member Author

J-N-K commented May 29, 2022

There is no topic. It's just '{ "TYPE" : "ALIVE" }', similar to the alive-event from the sitemap (which contains some additional information about the sitemap not needed here).

@spacemanspiff2007
Copy link
Contributor

Do I have to respond to this item with a rest call in any way?

@J-N-K
Copy link
Member Author

J-N-K commented May 30, 2022

No, the only reason is to detect a closed TCP connection (then the send will fail and the listener removed).

@lolodomo
Copy link
Contributor

lolodomo commented May 30, 2022 •
edited
Loading

@J-N-K : for the remote openHAB binding I believe this would require a simple additional case to cover the type ALIVE here:
https://github.com/openhab/openhab-addons/blob/main/bundles/org.openhab.binding.remoteopenhab/src/main/java/org/openhab/binding/remoteopenhab/internal/rest/RemoteopenhabRestClient.java#L416
Could you confirm?

@lolodomo
Copy link
Contributor

lolodomo commented May 30, 2022 •
edited
Loading

If we change nothing, the only risk is to have regular DEBUG logs mentioning ALIVE type messages not supported.
I can check that after this PR is merged and then propose the small enhancement to avoid these logs.

@spacemanspiff2007
Copy link
Contributor

No, the only reason is to detect a closed TCP connection (then the send will fail and the listener removed).

Wouldn't it only be necessary to send this some time after the last message was sent?

@J-N-K
Copy link
Member Author

J-N-K commented May 30, 2022

Technically yes, but that would require tracking which event made it to which subscription. Just keeping track of the last processed event globally is not sufficient, because of the filter it'll not reach all subscribers. IMO the burden of one extra event every 2 minutes does not justify more complex logic here.

@spacemanspiff2007
Copy link
Contributor

IMO the burden of one extra event every 2 minutes does not justify more complex logic here.

No worries - I thought the filters are connection bound and the connection knows when it's sending an event.
I'll have to support it anyway so I am fine with your proposed solution. 👍

@J-N-K
improvement
9b394e9 
Signed-off-by: Jan N. Klug <github@klug.nrw>
wborn
wborn approved these changes Jun 16, 2022
View reviewed changes
Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@wborn wborn merged commit 84d5d38 into openhab:main Jun 16, 2022
@wborn wborn added this to the 3.3 milestone Jun 16, 2022
@J-N-K J-N-K deleted the bug-fixdannglingssesubscriptions branch June 16, 2022 17:41
@lolodomo
Copy link
Contributor

If we change nothing, the only risk is to have regular DEBUG logs mentioning ALIVE type messages not supported. I can check that after this PR is merged and then propose the small enhancement to avoid these logs.

This is confirmed.

22:19:46.568 [DEBUG] [nternal.rest.RemoteopenhabRestClient] - Received unhandled event with name 'event' and data '{"type":"ALIVE"}'

I will have to propose a small update.

lolodomo added a commit to lolodomo/openhab-addons that referenced this pull request Jun 24, 2022
@lolodomo
[remoteopenhab] Avoid unexpected log for ALIVE SSE message
91d7c6f 
Related to openhab/openhab-core#2983

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo lolodomo mentioned this pull request Jun 24, 2022
Merged
@lolodomo
Copy link
Contributor

lolodomo commented Jun 25, 2022 •
edited
Loading

Just for my curiosity: all OH SSE events have "message" as name while your ALIVE event has "event". Is it expected?

openhab-core/bundles/org.openhab.core.io.rest.sse/src/main/java/org/openhab/core/io/rest/sse/internal/util/SseUtil.java

Line 54 in cdf876c

final OutboundSseEvent sseEvent = eventBuilder.name("message") //

@spacemanspiff2007 spacemanspiff2007 mentioned this pull request Jun 27, 2022
Closed
jlaur pushed a commit to openhab/openhab-addons that referenced this pull request Jun 27, 2022
@lolodomo
[remoteopenhab] Avoid unexpected log for ALIVE SSE message (#13008)
859a1eb 
Related to openhab/openhab-core#2983

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@J-N-K J-N-K mentioned this pull request Sep 19, 2022
Merged
leifbladt pushed a commit to leifbladt/openhab-addons that referenced this pull request Oct 15, 2022
@lolodomo @leifbladt
[remoteopenhab] Avoid unexpected log for ALIVE SSE message (openhab#1…
6415bd6 
…3008)

Related to openhab/openhab-core#2983

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
@lolodomo @andan67
[remoteopenhab] Avoid unexpected log for ALIVE SSE message (openhab#1…
2536cd7 
…3008)

Related to openhab/openhab-core#2983

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Nov 12, 2022
@lolodomo @andrasU
[remoteopenhab] Avoid unexpected log for ALIVE SSE message (openhab#1…
b4d9d6b 
…3008)

Related to openhab/openhab-core#2983

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Feb 23, 2023
@lolodomo @psmedley
[remoteopenhab] Avoid unexpected log for ALIVE SSE message (openhab#1…
3c19a9c 
…3008)

Related to openhab/openhab-core#2983

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Feb 28, 2023
@lolodomo @nemerdaud
[remoteopenhab] Avoid unexpected log for ALIVE SSE message (openhab#1…
5d3a8f4 
…3008)

Related to openhab/openhab-core#2983

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
@J-N-K @splatch
Fix dangling SSE subscriptions (openhab#2983)
7e4e8c2 
Unlike the sitemap SSE subscriptions generic event subscriptions did not implement a connection lost monitor.

Signed-off-by: Jan N. Klug <github@klug.nrw>
GitOrigin-RevId: 84d5d38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wborn wborn wborn approved these changes

Assignees
No one assigned
Labels
bug An unexpected problem or unintended behavior of the Core REST/SSE
Projects
None yet
Milestone
3.3
Development

Successfully merging this pull request may close these issues.

High memory usage & complete openHAB shutdown with many SSE subscriptions
4 participants
@J-N-K @lolodomo @spacemanspiff2007 @wborn