Don't hold a reference to the last job in the thread cache

[smurfix]

Fixes #1638

[codecov]

Codecov Report

Merging #1639 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1639   +/-   ##
=======================================
  Coverage   99.69%   99.69%           
=======================================
  Files         110      110           
  Lines       13894    13911   +17     
  Branches     1070     1070           
=======================================
+ Hits        13852    13869   +17     
  Misses         27       27           
  Partials       15       15           

Impacted Files	Coverage Δ
trio/_core/_thread_cache.py	100.00% <100.00%> (ø)
trio/_core/tests/test_thread_cache.py	100.00% <100.00%> (ø)

[belm0]

hard to add test coverage for it?

[smurfix]

Yes. I adapted the test_thread_cache_basics test:

def test_thread_cache_deref():
    res = [False]

    def test_main():
        class deltype(type):
            def __del__(self):
                res[0] = True

        Test = deltype("Test", (object,), {})
        def fn():
            return 42

        q = Queue()
        def deliver(outcome):
            q.put(outcome)

        start_thread_soon(fn, deliver)

        outcome = q.get()
        assert outcome.unwrap() == 42
        assert not res[0]
    test_main()
    gc_collect_harder()
    assert res[0]

which unfortunately fails to trigger the problem in the unpatched case.

[njsmith]

CI failure is the mysterious #1604, nothing related to this.

Nice catch, and we definitely want this. Can you add a test and newsfragment to the PR, though?

[belm0]

so this is the instance's destructor, where my original report was on the class destructor

but I see that the bug indeed affects both of these. (I suppose that not destroying the instance is enough to keep a reference to the class?)

does this variant of the test fail before the patch?

[belm0]

does this variant of the test fail before the patch?

I see it does 👍

[smurfix]

I couldn't easily reproduce the problem using a class destructor, so I opted for a minimal test that shows the reference to be gone / some destructor to be called.