Run GC on PyPy more for test_worker_thread_context_not_leaked

[A5rocks]

Fixes #3478

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00000%. Comparing base (5249b54) to head (33e2594).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@               Coverage Diff               @@
##                 main        #3479   +/-   ##
===============================================
  Coverage   100.00000%   100.00000%           
===============================================
  Files             128          128           
  Lines           19450        19450           
  Branches         1320         1321    +1     
===============================================
  Hits            19450        19450           

Files with missing lines	Coverage Δ
src/trio/_core/_thread_cache.py	100.00000% <100.00000%> (ø)
src/trio/_tests/test_threads.py	100.00000% <100.00000%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[EmmanuelNiyonshuti]

@A5rocks Sorry, I thought it would have no issue on PyPy. let me check again.

[A5rocks]

Yes if you have an Ubuntu (presumably kinda slow) machine and could check that this fixes the issue (ie repro w/o + unable to repro with) that would be great.

I'll start a bunch of CI runs in a while otherwise, which will hopefully just provide the same evidence.

[EmmanuelNiyonshuti]

Yes if you have an Ubuntu (presumably kinda slow) machine and could check that this fixes the issue (ie repro w/o + unable to repro with) that would be great.

I'll start a bunch of CI runs in a while otherwise, which will hopefully just provide the same evidence.

I think I have that; will try and see.

[EmmanuelNiyonshuti]

I am failing to reproduce its flakiness on my host.

[EmmanuelNiyonshuti]

I think this is better. I wasn't aware of trio's gc_collect_harder!

[A5rocks]

With some extra changes + without that change: https://github.com/A5rocks/trio/actions/runs/30357228537

With some extra changes (same as above) + with that change: https://github.com/A5rocks/trio/actions/runs/30357043985

So I think it's necessary.

[EmmanuelNiyonshuti]

With some extra changes + without that change: https://github.com/A5rocks/trio/actions/runs/30357228537

With some extra changes (same as above) + with that change: https://github.com/A5rocks/trio/actions/runs/30357043985

So I think it's necessary.

you didn't add an else block though? gc.collect

[EmmanuelNiyonshuti]

With some extra changes + without that change: https://github.com/A5rocks/trio/actions/runs/30357228537
With some extra changes (same as above) + with that change: https://github.com/A5rocks/trio/actions/runs/30357043985
So I think it's necessary.

you didn't add an else block though? gc.collect

perhaps is related to "extra changes," which I don't quite understand yet!

[A5rocks]

No else block is necessary, because there's no refcycles actually. The CPython failures turned out to be a race condition where the worker thread would sometimes not get to return (which would decrease the refcount for contextvars, cleaning them up), which calling gc.collect() only solved by giving the worker thread more time to return.

[EmmanuelNiyonshuti]

No else block is necessary, because there's no refcycles actually. The CPython failures turned out to be a race condition where the worker thread would sometimes not get to return (which would decrease the refcount for contextvars, cleaning them up), which calling gc.collect() only solved by giving the worker thread more time to return.

oh! will that also work on free threaded python, assuming you mean default CPython?

[A5rocks]

I don't see why it wouldn't! I don't think free threaded CPython is that different from regular CPython in this case (we only actually notify of task completion once the refcount is decreased).

[EmmanuelNiyonshuti]

I don't see why it wouldn't! I don't think free threaded CPython is that different from regular CPython in this case (we only actually notify of task completion once the refcount is decreased).

hmm... I see! so you somehow found a way to delay/make sure worker thread returns? Obviously, I wasn't able to reproduce this on CPython, so I kinda narrowed the problem to free threaded,

[EmmanuelNiyonshuti]

I don't see why it wouldn't! I don't think free threaded CPython is that different from regular CPython in this case (we only actually notify of task completion once the refcount is decreased).

hmm... I see! so you somehow found a way to delay/make sure worker thread returns? Obviously, I wasn't able to reproduce this on CPython, so I kinda narrowed the problem to free threaded,

FYI the test is still flaky on free threaded. you can try running it couple times.

[A5rocks]

No, the worker thread already sends a message about being done (deliver in _handle_job), all I did was make sure it removed its reference to worker_fn (which keeps a strong ref on a copy of contextvars, keeping existing contextvars alive) before saying it's done -- this guarantees that when the await to_thread.run_sync finishes, the copy of the contextvars has been deleted.

[EmmanuelNiyonshuti]

I don't see why it wouldn't! I don't think free threaded CPython is that different from regular CPython in this case (we only actually notify of task completion once the refcount is decreased).

The difference I think we're dealing with is refcounting https://docs.python.org/3/howto/free-threading-python.html#:~:text=Because%20the%20per,to%20be%20freed.

[EmmanuelNiyonshuti]

No, the worker thread already sends a message about being done (deliver in _handle_job), all I did was make sure it removed its reference to worker_fn (which keeps a strong ref on a copy of contextvars, keeping existing contextvars alive) before saying it's done -- this guarantees that when the await to_thread.run_sync finishes, the copy of the contextvars has been deleted.

hmm... that's more interesting!