gh-74028: concurrent.futures.Executor.map: avoid reference cycles when an exception is raised
#131701
Conversation
…le from failed future captured in its exception’s traceback
ebonnal
changed the title
test_concurrent_futures.test_map_exception: assert no reference cycle from failed future captured in its exception’s tracebacktest_concurrent_futures: Executor.map: assert no reference cycle from failed future captured in its exception’s traceback
ebonnal
changed the title
test_concurrent_futures: Executor.map: assert no reference cycle from failed future captured in its exception’s tracebackconcurrent.futures.Executor.map test: assert no reference cycle from failed future captured in its exception’s traceback
ebonnal
changed the title
concurrent.futures.Executor.map test: assert no reference cycle from failed future captured in its exception’s tracebackconcurrent.futures.Executor.map: test no reference cycle from failed future captured in its exception’s traceback
|
only failing for Ubuntu (free-threading) / build and test (ubuntu-24.04-arm) and I cannot reproduce on my ubuntu arm VM Edit: actually I did reproduce it after a couple attempts |
picnixz
changed the title
concurrent.futures.Executor.map: test no reference cycle from failed future captured in its exception’s tracebackconcurrent.futures.Executor.map: test no reference cycle from failed future captured in its exception’s traceback
|
@ebonnal thanks for the test! Your first test was good, and it found a bug https://github.com/python/cpython/actions/runs/14048391667/job/39333996238?pr=131701#step:22:632 I've pushed a commit to revert to the original style of test - which includes the references in the error message, and fixed the bug. |
ebonnal
changed the title
concurrent.futures.Executor.map: test no reference cycle from failed future captured in its exception’s tracebackconcurrent.futures.Executor.map: test there are no reference cycles when an exception is raised
… to an Exception or a failed Future
Thanks @graingert !
Nice, I have cleaned up this (I have renamed the PR to show that we have extended its scope) |
|
ok I can't repeat it locally, even disabling the GC - sorry for hijacking your PR but I think I need to test it in CI to repeat |
|
Hey @graingert, update from my investigations with a free-threading build on an Ubuntu ARM machine: |
|
I don't think it's a gc race condition as the problem still happens with the GC disabled. I think the problem is that the main thread is resumed before the background thread can delete the future. I'm happy for you to revert back to 03f8ab4 |
|
Clear, thanks for the help @graingert, I appreciate it! Merging this test will definitely help prevent future regressions (e.g. in #131467). Should we open a follow up issue summarizing the behavior we have found with free-threading builds on some platforms? Happy to do it! |
|
@graingert ready for your review 🙏🏻 |
ebonnal
changed the title
concurrent.futures.Executor: avoid reference cycles when an exception is raisedconcurrent.futures.Executor.map: avoid reference cycles when an exception is raised
A failed
futurestores the raise exception in its._exceptionattribute. We get a reference cycle if this exception's__traceback__refers back to thefuture.A particular care has been taken to avoid that (e.g. by @graingert in #95169)
This PR tests this behavior.
(follows up discussion in #131467)
Edit:
We extend this PR to ensure that no reference to an
Exceptionis captured in the error's traceback frames.