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

[chromecast] Initialize connection to devices asynchronously #9228

Merged
merged 1 commit into from
Apr 9, 2021

Conversation

cweitkamp
Copy link
Contributor

  • Initialize connection to devices asynchronously to avoid slow initialization
2020-12-04 08:34:25.273 [WARN ] [core.thing.internal.ThingManagerImpl] - Initializing handler for thing 'chromecast:chromecast:cc4c584bfcc8ccd3ab25986161374e62' takes more than 5000ms.

Signed-off-by: Christoph Weitkamp [email protected]

Comment on lines 99 to 115
scheduler.submit(() -> {
// initialize connection to devices asynchronously
Coordinator localCoordinator = coordinator;
if (localCoordinator != null && (!localCoordinator.chromeCast.getAddress().equals(ipAddress)
|| localCoordinator.chromeCast.getPort() != config.port)) {
localCoordinator.destroy();
localCoordinator = coordinator = null;
}

if (localCoordinator == null) {
ChromeCast chromecast = new ChromeCast(ipAddress, config.port);
localCoordinator = new Coordinator(this, thing, chromecast, config.refreshRate, audioHTTPServer,
callbackUrl);
localCoordinator.initialize();
coordinator = localCoordinator;
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I think quickly cycling the handler (initialize->dispose->initialize) would cause multiple Coordinator instances to be created simultaneously and also lose track of one of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Calling the coordinator.initialize() method asynchronously and keep the instance creating like before should work either.

Copy link
Contributor

@cpmeister cpmeister Dec 7, 2020

Choose a reason for hiding this comment

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

Only if coordinator.initialize() is synchronized along with coordinator.destroy() as well as adding an early exit in coordinator.initialize() if coordinator.destroy() was called first.

I think that would make it thread safe.

Copy link
Member

Choose a reason for hiding this comment

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

@cweitkamp Do you have this PR still on your radar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kaikreuzer Thanks for the reminder. I updated the PR with the suggested solution above.

@fwolter fwolter added the awaiting feedback Awaiting feedback from the pull request author label Mar 3, 2021
@cweitkamp cweitkamp force-pushed the bugfix-chromecast-initialize branch from 980e97e to d629b55 Compare April 9, 2021 10:38
@kaikreuzer kaikreuzer removed the awaiting feedback Awaiting feedback from the pull request author label Apr 9, 2021
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Thanks, lgtm.

@kaikreuzer kaikreuzer added this to the 3.1 milestone Apr 9, 2021
@kaikreuzer kaikreuzer added the enhancement An enhancement or new feature for an existing add-on label Apr 9, 2021
@kaikreuzer kaikreuzer merged commit 1822f77 into openhab:main Apr 9, 2021
@cweitkamp cweitkamp deleted the bugfix-chromecast-initialize branch April 9, 2021 20:07
themillhousegroup pushed a commit to themillhousegroup/openhab2-addons that referenced this pull request May 10, 2021
Signed-off-by: Christoph Weitkamp <[email protected]>
Signed-off-by: John Marshall <[email protected]>
computergeek1507 pushed a commit to computergeek1507/openhab-addons that referenced this pull request Jul 13, 2021
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants