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

Clear worker thread cache after forking #3200

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

A5rocks
Copy link
Contributor

@A5rocks A5rocks commented Feb 5, 2025

Fixes #2764

While we don't provide any sort of guarantees and I think it's tempting to give up on this:

  • Python (3.12+) will already warn on this, so they have that handled
  • It's friendlier to do the right thing.

As long as people aren't initializing their own thread caches, this should be fine.

@A5rocks
Copy link
Contributor Author

A5rocks commented Feb 5, 2025

Maybe there should be a way to tell Trio "hey I don't want the worker threads anymore, please kill them!", especially in light of the new deprecation warning (ATM the way to do that is to do time.sleep(10)). Maybe Trio should explicitly support this use case and set up a callback for before os.fork() s.t. all the threads get killed.

That's not what this PR does!

Counterpoint: what if a thread is currently doing work (no guarantees even outside of a trio.run because abandon_on_cancel).

Copy link

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00000%. Comparing base (8a7674c) to head (0855d7c).

Additional details and impacted files
@@               Coverage Diff               @@
##                 main        #3200   +/-   ##
===============================================
  Coverage   100.00000%   100.00000%           
===============================================
  Files             124          124           
  Lines           18792        18818   +26     
  Branches         1268         1270    +2     
===============================================
+ Hits            18792        18818   +26     
Files with missing lines Coverage Δ
src/trio/_core/_tests/test_thread_cache.py 100.00000% <100.00000%> (ø)
src/trio/_core/_thread_cache.py 100.00000% <100.00000%> (ø)

@jakkdl
Copy link
Member

jakkdl commented Feb 5, 2025

coverage fail appears to be nedbat/coveragepy#310
seems to be workarounds in there, though this is minor enough we could ofc just # pragma: no cover

Copy link
Member

@CoolCat467 CoolCat467 left a comment

Choose a reason for hiding this comment

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

Not sure if I completely understand the test for this, but the actual functionality changes look fine.

@A5rocks A5rocks requested a review from jakkdl February 5, 2025 23:54
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.

Clear global THREAD_CACHE in child after fork()
3 participants