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

[mycroft] Adjust thread name for web socket client #14342

Merged
merged 1 commit into from
Feb 19, 2023
Merged

[mycroft] Adjust thread name for web socket client #14342

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 [email protected]

@lolodomo lolodomo requested a review from dalgwen as a code owner February 5, 2023 10:35
dalgwen
dalgwen approved these changes Feb 5, 2023
View reviewed changes
Copy link
Contributor

@dalgwen dalgwen left a comment

Choose a reason for hiding this comment

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

👍

@lolodomo
Copy link
Contributor Author

lolodomo commented Feb 5, 2023 •
edited
Loading

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

I see in another binding that hashCode() is used. Maybe a better idea. I am asking myself what is the length of the value returned by hashCode.

@lolodomo lolodomo added the work in progress A PR that is not yet ready to be merged label Feb 5, 2023
@lolodomo lolodomo force-pushed the mycroft_http_threadname branch from 94b5e6d to 1c508a9 Compare February 5, 2023 17:13
@lolodomo lolodomo removed 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

@dalgwen : please have a new look.

@lolodomo lolodomo force-pushed the mycroft_http_threadname branch from 1c508a9 to 03e701e Compare February 5, 2023 18:01
@lolodomo lolodomo changed the title [mycroft] Adjust thread name when thing UID can't be used [mycroft] 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
dalgwen
dalgwen approved these changes Feb 6, 2023
View reviewed changes
Copy link
Contributor

@dalgwen dalgwen left a comment

Choose a reason for hiding this comment

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

OK for me.
But the whole websocket id construction really seems to be some kind of black magic.
I see that you made some merge request on other bindings for the same purpose.
Shouldn't it be a common method somewhere ?
Like in HexUtils or in UIDUtils ?
It is so easy to not using the thread name properly.

@lolodomo
Copy link
Contributor Author

lolodomo commented Feb 7, 2023

I see that you made some merge request on other bindings for the same purpose.
Shouldn't it be a common method somewhere ?
Like in HexUtils or in UIDUtils ?
It is so easy to not using the thread name properly.

Could make sense to add a method in the same class as the one that provides createHttpClient and createWebSocketClient.

@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 force-pushed the mycroft_http_threadname branch from 03e701e to 8845a51 Compare February 7, 2023 20:24
@lolodomo lolodomo changed the title [mycroft] Adjust thread name when thing UID can't be used (avoid IAE) [mycroft] Adjust thread name for web socket client Feb 7, 2023
@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 lolodomo force-pushed the mycroft_http_threadname branch from 8845a51 to 6fba25f Compare February 19, 2023 18:20
@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 fa87a1b into openhab:main Feb 19, 2023
@wborn wborn added this to the 4.0 milestone Feb 19, 2023
@lolodomo lolodomo deleted the mycroft_http_threadname branch February 19, 2023 19:59
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Feb 28, 2023
@lolodomo @nemerdaud
[mycroft] Adjust thread name for web socket client (openhab#14342)
790be72 
Signed-off-by: Laurent Garnier <[email protected]>
renescherer pushed a commit to renescherer/openhab-addons that referenced this pull request Mar 23, 2023
@lolodomo @renescherer
[mycroft] Adjust thread name for web socket client (openhab#14342)
609df0a 
Signed-off-by: Laurent Garnier <[email protected]>
FordPrfkt pushed a commit to FordPrfkt/openhab-addons that referenced this pull request Apr 20, 2023
@lolodomo @FordPrfkt
[mycroft] Adjust thread name for web socket client (openhab#14342)
9cf5e71 
Signed-off-by: Laurent Garnier <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalgwen dalgwen dalgwen approved these changes

@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 @dalgwen @wborn