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

[denonmarantz] Fix blocking initialization #17057

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

jlaur
Copy link
Contributor

@jlaur jlaur commented Jul 14, 2024

Resolves #16806

@jlaur jlaur added the bug An unexpected problem or unintended behavior of an add-on label Jul 14, 2024
@jlaur jlaur requested a review from jwveldhuis as a code owner July 14, 2024 21:41
@jlaur jlaur added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jul 14, 2024
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Thanks, very clean.

I would consider this for backporting, agree?

@jlaur
Copy link
Contributor Author

jlaur commented Jul 14, 2024

I would consider this for backporting, agree?

Probably not, since it's not zero-risk, and I don't think it's critical - it's been like this for years. So I'd like to run it on my production system for some time to confirm there are no side-effects. I successfully tested two scenarios briefly:

  1. Adding a Thing with non-existing IP address to reproduce timeouts/failing auto-configuration.
  2. Initializing a working Thing for my receiver (i.e. skipping auto-configuration).

@jlaur jlaur marked this pull request as draft July 14, 2024 22:07
@jlaur
Copy link
Contributor Author

jlaur commented Jul 14, 2024

@lsiepel - and now I realized I need to check what happens in case of dispose before the auto-configuration completes.

@lsiepel
Copy link
Contributor

lsiepel commented Jul 14, 2024

@lsiepel - and now I realized I need to check what happens in case of dispose before the auto-configuration completes.

I guess you want to catch the interruption exception and quit/return immediate and not start the second telnet connection

Resolves openhab#16806

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur jlaur force-pushed the 16806-denonmarantz-initialize-async branch from 14c070e to 2d7fbc9 Compare July 14, 2024 22:31
@jlaur
Copy link
Contributor Author

jlaur commented Jul 14, 2024

@lsiepel - and now I realized I need to check what happens in case of dispose before the auto-configuration completes.

I guess you want to catch the interruption exception and quit/return immediate and not start the second telnet connection

InterruptedException is only thrown when the thread is interrupted. 😉 The issus is it was wild-running without a handle, and as a result "Handler DenonMarantzHandler tried updating the thing status although the handler was already disposed." and it kept trying to connect after it should have been disposed. I will need to check exactly why, because something else might also be going on.

I now did a change so the initialization job is scheduled and can be interrupted. It seems this also resolved the issue with endless reconnects. I will do a few more tests and checks tomorrow, so keeping as draft for now.

@jlaur
Copy link
Contributor Author

jlaur commented Jul 15, 2024

I now did a change so the initialization job is scheduled and can be interrupted. It seems this also resolved the issue with endless reconnects. I will do a few more tests and checks tomorrow, so keeping as draft for now.

I had another look to figure out what was going on when creating a Thing with invalid IP address and quickly deleting the Thing again. It was more or less as expected: The initialization thread was not interrupted by dispose. Thus the initialization thread would call createConnection after dispose had called connector.dispose(). So indeed it is resolved with the change I made yesterday.

@jlaur jlaur marked this pull request as ready for review July 15, 2024 10:40
@lsiepel lsiepel merged commit 2f35a79 into openhab:main Jul 15, 2024
5 checks passed
@lsiepel lsiepel added this to the 4.3 milestone Jul 15, 2024
@jlaur jlaur deleted the 16806-denonmarantz-initialize-async branch July 17, 2024 14:04
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Jul 18, 2024
Resolves openhab#16806

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
digitaldan pushed a commit to digitaldan/openhab-addons that referenced this pull request Aug 29, 2024
Resolves openhab#16806

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
pgfeller pushed a commit to pgfeller/openhab-addons that referenced this pull request Sep 29, 2024
Resolves openhab#16806

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
Resolves openhab#16806

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Oct 18, 2024
Resolves openhab#16806

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
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.

[denonmarantz] Auto-configuration is blocking initialize
2 participants