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

[wemo] Fix stability issues #14163

Merged
merged 3 commits into from
Jan 28, 2023
Merged

[wemo] Fix stability issues #14163

merged 3 commits into from
Jan 28, 2023

Conversation

jlaur
Copy link
Contributor

@jlaur jlaur commented Jan 5, 2023 •
edited
Loading

Fixes #14162.

Tested on my system for a few weeks now and didn't see any blocked threads since. Also my system in general is now stable and responsive after running for more than a week without restarts.

JARs for testing:

@jlaur jlaur added the bug An unexpected problem or unintended behavior of an add-on label Jan 5, 2023
@jlaur jlaur force-pushed the 14162-wemo-stability branch 2 times, most recently from 45d9fd3 to 40759a6 Compare January 11, 2023 16:05
@jlaur jlaur mentioned this pull request Jan 11, 2023
Merged
@jlaur jlaur force-pushed the 14162-wemo-stability branch from 40759a6 to e35cfe4 Compare January 13, 2023 22:24
@jlaur
Copy link
Contributor Author

jlaur commented Jan 14, 2023

@hmerk - I don't remember if I've asked you before, sorry: Do you know how to run the integrations tests locally? With:

mvn -pl :org.openhab.binding.wemo.tests clean install -DwithResolver=true

I currently end up with:

[INFO] Resolving openhab-addons\itests\org.openhab.binding.wemo.tests\itest.bndrun:
[ERROR] Resolution failed. Capabilities satisfying the following requirements could not be found:
    [<<INITIAL>>]
      ? osgi.identity: (osgi.identity=org.openhab.core.binding.xml)
    [org.openhab.binding.wemo.tests version=4.0.0.202301132049 type=osgi.fragment]
      ? osgi.wiring.package: (osgi.wiring.package=org.openhab.core.common.registry)

@hmerk
Copy link
Contributor

hmerk commented Jan 14, 2023

@jlaur Sorry, don‘t know about the tests.
@J-N-K can you help?

@jlaur
Stabilize UPnP subscription handling
d78f553 
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur jlaur force-pushed the 14162-wemo-stability branch from e35cfe4 to d78f553 Compare January 24, 2023 23:08
jlaur added 2 commits January 25, 2023 00:17
@jlaur
Remove unused UpnpService
cab77c4 
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur
Fix integration test
30db45f 
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur jlaur force-pushed the 14162-wemo-stability branch from a2133bd to 30db45f Compare January 24, 2023 23:24
@jlaur jlaur marked this pull request as ready for review January 24, 2023 23:28
@jlaur jlaur requested a review from hmerk as a code owner January 24, 2023 23:28
@jlaur jlaur requested a review from a team January 25, 2023 08:35
@jlaur
Copy link
Contributor Author

jlaur commented Jan 25, 2023

@openhab/add-ons-maintainers - this should be considered for being cherry-picked into 3.4.x.

hmerk
hmerk approved these changes Jan 25, 2023
View reviewed changes
Copy link
Contributor

@hmerk hmerk left a comment

Choose a reason for hiding this comment

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

LGTM

@jlaur
Copy link
Contributor Author

jlaur commented Jan 27, 2023

@lolodomo - can you have a look at this?

lolodomo
lolodomo approved these changes Jan 28, 2023
View reviewed changes
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.
PS: I checked the Sonos binding and a lock object is also used ;)

@lolodomo lolodomo merged commit 0e65903 into openhab:main Jan 28, 2023
@lolodomo lolodomo added this to the 4.0 milestone Jan 28, 2023
@jlaur
Copy link
Contributor Author

jlaur commented Jan 28, 2023

@lolodomo - thanks for the review. Can I cherry-pick into 3.4.x?

@jlaur jlaur deleted the 14162-wemo-stability branch January 28, 2023 09:24
@jlaur
Copy link
Contributor Author

jlaur commented Jan 28, 2023

PS: I checked the Sonos binding and a lock object is also used ;)

I also compared with the Sonos implementation. I basically did three things:

  • Removed the unneeded renewal complexity.
  • Added locking.
  • Fixed a small problem in core.

At least it's working really well on my system now, and it's still stable and responsive after weeks. Very new situation for me. 🙂

@jaywiseman1971
Copy link

Is the top post the latest version of the JAR if not, can I get a link to the JAR?

I have 37 WeMo's in my setup that I liked to test. I'm stilling using Han's version from 1/2022.

Best, Jay

@jlaur
Copy link
Contributor Author

jlaur commented Jan 28, 2023

@jaywiseman1971 - yes, you can use one of the provided JAR's mentioned in the description. They were built before the last two commits, but this should not matter much. Tomorrow there will be a fully up-to-date 4.0 snapshot version.

@jaywiseman1971
Copy link

@jaywiseman1971 - yes, you can use one of the provided JAR's mentioned in the description. They were built before the last two commits, but this should not matter much. Tomorrow there will be a fully up-to-date 4.0 snapshot version.

Thank you! So far so good! Will report back. FYI, I'm running OH 3.2.

Best, Jay

@lolodomo
Copy link
Contributor

@lolodomo - thanks for the review. Can I cherry-pick into 3.4.x?

Would make sense.
I have not in mind other concurrent PR merged for this binding.

@jlaur jlaur mentioned this pull request Jan 29, 2023
Merged
jlaur added a commit that referenced this pull request Jan 29, 2023
@jlaur
[wemo] Fix stability issues (#14163)
da15059 
* Stabilize UPnP subscription handling
* Remove unused UpnpService
* Fix integration test

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur jlaur added the patch A PR that has been cherry-picked to a patch release branch label Jan 29, 2023
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/wemo-reliability-3-x-version-release/143897/1

psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Feb 23, 2023
@jlaur @psmedley
[wemo] Fix stability issues (openhab#14163)
a67654a 
* Stabilize UPnP subscription handling
* Remove unused UpnpService
* Fix integration test

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Feb 28, 2023
@jlaur @nemerdaud
[wemo] Fix stability issues (openhab#14163)
811e991 
* Stabilize UPnP subscription handling
* Remove unused UpnpService
* Fix integration test

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
FordPrfkt pushed a commit to FordPrfkt/openhab-addons that referenced this pull request Apr 20, 2023
@jlaur @FordPrfkt
[wemo] Fix stability issues (openhab#14163)
7da73ff 
* Stabilize UPnP subscription handling
* Remove unused UpnpService
* Fix integration test

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur jlaur mentioned this pull request Sep 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lolodomo lolodomo lolodomo approved these changes

@hmerk hmerk hmerk approved these changes

Assignees
No one assigned
Labels
bug An unexpected problem or unintended behavior of an add-on patch A PR that has been cherry-picked to a patch release branch
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.

[wemo] Stability issues affecting openHAB as a whole
5 participants
@jlaur @hmerk @jaywiseman1971 @lolodomo @openhab-bot