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 4 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

Attention: Patch coverage is 85.71429% with 3 lines in your changes missing coverage. Please review.

Project coverage is 99.98405%. Comparing base (8a7674c) to head (b146087).

Files with missing lines Patch % Lines
src/trio/_core/_tests/test_thread_cache.py 87.50000% 1 Missing and 1 partial ⚠️
src/trio/_core/_thread_cache.py 80.00000% 1 Missing ⚠️
Additional details and impacted files
@@                 Coverage Diff                  @@
##                 main       #3200         +/-   ##
====================================================
- Coverage   100.00000%   99.98405%   -0.01596%     
====================================================
  Files             124         124                 
  Lines           18792       18813         +21     
  Branches         1268        1270          +2     
====================================================
+ Hits            18792       18810         +18     
- Misses              0           2          +2     
- Partials            0           1          +1     
Files with missing lines Coverage Δ
src/trio/_core/_thread_cache.py 98.88889% <80.00000%> (-1.11111%) ⬇️
src/trio/_core/_tests/test_thread_cache.py 98.46154% <87.50000%> (-1.53847%) ⬇️

@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

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()
2 participants