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

[deconz] Adjust thread name for web socket client #14343

Merged
merged 1 commit into from
Feb 19, 2023
Merged

[deconz] Adjust thread name for web socket client #14343

merged 1 commit into from
Feb 19, 2023

Conversation

lolodomo
Copy link
Contributor

@lolodomo lolodomo commented Feb 5, 2023 •
edited
Loading

Signed-off-by: Laurent Garnier lg.hc@free.fr

@lolodomo lolodomo requested a review from a team as a code owner February 5, 2023 10:51
@lolodomo
Copy link
Contributor Author

lolodomo commented Feb 5, 2023

A counter is maybe not the best option because a new thread pool will be created each time the thing handler is initialized.

@lolodomo lolodomo added the work in progress A PR that is not yet ready to be merged label Feb 5, 2023
@lolodomo
Copy link
Contributor Author

lolodomo commented Feb 5, 2023

Whatever the provided name, a new thread pool is created by OH core framework.
Having thread pools with same name is probably allowed.

In case thing UID is too big, we could first try with the thing ID.

PS: I am asking myself if the created thread pool is released when the web socket client is abandoned.

@lolodomo lolodomo force-pushed the deconz_http_threadname branch from 4815be1 to 8de6863 Compare February 5, 2023 17:09
@lolodomo lolodomo removed the work in progress A PR that is not yet ready to be merged label Feb 5, 2023
J-N-K
J-N-K reviewed Feb 5, 2023
View reviewed changes
...ng.deconz/src/main/java/org/openhab/binding/deconz/internal/handler/DeconzBridgeHandler.java Outdated
websocketID = "deconz-" + thing.getUID().getId();
}
if (websocketID.length() > 20) {
websocketID = "deconz";
Copy link
Member

Choose a reason for hiding this comment

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

That's not a good idea. It should be unique, but repeatable. Maybe hash the full ID and take the last 6 bytes as hex (12 characters + deconz- prefix makes a total of 19 characters). I doubt that we'll run into collisions with that and it's nicely reproducible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is better if it is unique but not required.
But your idea consisting in hashing the UID is good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait a second...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is ok now

@lolodomo lolodomo force-pushed the deconz_http_threadname branch from d51f766 to d83deab Compare February 5, 2023 17:58
@lolodomo lolodomo changed the title [deconz] Adjust thread name when thing UID can't be used [deconz] Adjust thread name when thing UID can't be used (avoid IAE) Feb 5, 2023
@lolodomo lolodomo added the bug An unexpected problem or unintended behavior of an add-on label Feb 5, 2023
@lolodomo
Copy link
Contributor Author

lolodomo commented Feb 7, 2023

Awaiting a new method in core framework.

@lolodomo lolodomo added the awaiting other PR Depends on another PR label Feb 7, 2023
@lolodomo
Copy link
Contributor Author

lolodomo commented Feb 7, 2023

Awaiting PR openhab/openhab-core#3359 to be merged to then update this PR

@lolodomo lolodomo added the work in progress A PR that is not yet ready to be merged label Feb 7, 2023
@lolodomo lolodomo changed the title [deconz] Adjust thread name when thing UID can't be used (avoid IAE) [deconz] Adjust thread name for web socket client Feb 7, 2023
@lolodomo lolodomo force-pushed the deconz_http_threadname branch from d83deab to 9e5c5b2 Compare February 7, 2023 20:28
@lolodomo lolodomo added enhancement An enhancement or new feature for an existing add-on and removed bug An unexpected problem or unintended behavior of an add-on labels Feb 7, 2023
@lolodomo
Copy link
Contributor Author

lolodomo commented Feb 7, 2023

My initial analysis was wrong, there is no risk of IAE with the current code.

@lolodomo
[deconz] Adjust thread name for web socket client
c5453d1 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo lolodomo force-pushed the deconz_http_threadname branch from 9e5c5b2 to c5453d1 Compare February 19, 2023 18:27
@lolodomo lolodomo removed work in progress A PR that is not yet ready to be merged awaiting other PR Depends on another PR labels Feb 19, 2023
@wborn wborn merged commit b0eaa9e into openhab:main Feb 19, 2023
@wborn wborn added this to the 4.0 milestone Feb 19, 2023
@lolodomo lolodomo deleted the deconz_http_threadname branch February 19, 2023 19:58
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Feb 28, 2023
@lolodomo @nemerdaud
[deconz] Adjust thread name for web socket client (openhab#14343)
804dd02 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@jlaur jlaur mentioned this pull request Mar 17, 2023
Merged
renescherer pushed a commit to renescherer/openhab-addons that referenced this pull request Mar 23, 2023
@lolodomo @renescherer
[deconz] Adjust thread name for web socket client (openhab#14343)
e874f9a 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
FordPrfkt pushed a commit to FordPrfkt/openhab-addons that referenced this pull request Apr 20, 2023
@lolodomo @FordPrfkt
[deconz] Adjust thread name for web socket client (openhab#14343)
55ec39a 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@J-N-K J-N-K J-N-K left review comments

@wborn wborn wborn approved these changes

Assignees
No one assigned
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@lolodomo @J-N-K @wborn