Skip to content

Explicit add the timeout thread to default ThreadGroup #22

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

Merged
merged 1 commit into from
Sep 27, 2022

Conversation

larskanis
Copy link
Contributor

Otherwise the timeout thread would be added to the ThreadGroup of the thread that makes the first call to Timeout.timeout .

Fixes bug 19020: https://bugs.ruby-lang.org/issues/19020

Do you wish a test for the ThreadGroup or is it an implementation detail?

@eregon
Copy link
Member

eregon commented Sep 26, 2022

Yes, a test would be very useful to ensure there is no regression.

@larskanis
Copy link
Contributor Author

I added a test case.

Otherwise the timeout thread would be added to the ThreadGroup of the thread that makes the first call to Timeout.timeout .

Fixes bug 19020: https://bugs.ruby-lang.org/issues/19020

Add a test case to make sure the common thread doesn't leak to another ThreadGroup
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!

@eregon eregon merged commit c4f1385 into ruby:master Sep 27, 2022
@eregon
Copy link
Member

eregon commented Sep 27, 2022

@hsbt How do we make sure this change is propagated to ruby/ruby before the 3.2.0 release (and ideally before the next preview too) ?

@eregon
Copy link
Member

eregon commented Sep 27, 2022

Oh wow it's done automatically, awesome! ruby/ruby@9d56d99

@larskanis larskanis deleted the timeout-thread branch September 28, 2022 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants