Skip to content

Explicit add the timeout thread to default ThreadGroup#22

Merged
eregon merged 1 commit into
ruby:masterfrom
larskanis:timeout-thread
Sep 27, 2022
Merged

Explicit add the timeout thread to default ThreadGroup#22
eregon merged 1 commit into
ruby:masterfrom
larskanis:timeout-thread

Conversation

@larskanis

Copy link
Copy Markdown
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

eregon commented Sep 26, 2022

Copy link
Copy Markdown
Member

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

@larskanis

Copy link
Copy Markdown
Contributor Author

I added a test case.

Comment thread test/test_timeout.rb Outdated
Comment thread test/test_timeout.rb Outdated
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

@eregon eregon left a comment

Copy link
Copy Markdown
Member

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

eregon commented Sep 27, 2022

Copy link
Copy Markdown
Member

@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

eregon commented Sep 27, 2022

Copy link
Copy Markdown
Member

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