gh-74028: concurrent.futures.Executor.map: avoid reference cycles when an exception is raised#131701
Uh oh!
There was an error while loading. Please reload this page.
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 tracebackebonnal 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 tracebackebonnal 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 tracebackonly 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
Uh oh!
There was an error while loading. Please reload this page.
@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. |
Uh oh!
There was an error while loading. Please reload this page.
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
Thanks @graingert !
Nice, I have cleaned up this (I have renamed the PR to show that we have extended its scope) |
graingert commented Apr 3, 2025
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: |
graingert commented Apr 7, 2025
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 |
ebonnal commented Apr 7, 2025
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! |
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 raisedready for your final review fyi @graingert 🙏🏻 |
| self.assertEqual(i.__next__(), (0, 1)) | ||
| self.assertEqual(i.__next__(), (0, 1)) | ||
| with self.assertRaises(ZeroDivisionError): | ||
There was a problem hiding this comment.
This test looks fragile to me, +1 to remove test altogether, the fix is straightforward enough
4 participants
A failed
futurestores the raised 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.