GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-74028: `concurrent.futures.Executor.map`: avoid reference cycles when an exception is raised by ebonnal · Pull Request #131701 · python/cpython

Conversation

@ebonnal

@ebonnal ebonnal commented Mar 24, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

A failed future stores the raised exception in its ._exception attribute. We get a reference cycle if this exception's __traceback__ refers back to the future.

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 Exception is captured in the error's traceback frames.

  • Issue: Make Executor.map work with infinite/large inputs correctly #74028

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added the awaiting review label Mar 24, 2025
@ebonnal ebonnal mentioned this pull request Mar 24, 2025
gh-131466: concurrent.futures.Executor.map: avoid temporarily exceeding buffersize while collecting the next result #131467
Open
@ebonnal ebonnal changed the title test_concurrent_futures.test_map_exception: assert no reference cycle from failed future captured in its exception’s traceback test_concurrent_futures: Executor.map: assert no reference cycle from failed future captured in its exception’s traceback Mar 25, 2025
@ebonnal ebonnal changed the title test_concurrent_futures: Executor.map: assert no reference cycle from failed future captured in its exception’s traceback concurrent.futures.Executor.map test: assert no reference cycle from failed future captured in its exception’s traceback Mar 25, 2025
@ebonnal ebonnal changed the title concurrent.futures.Executor.map test: assert no reference cycle from failed future captured in its exception’s traceback concurrent.futures.Executor.map: test no reference cycle from failed future captured in its exception’s traceback Mar 25, 2025
@ebonnal

ebonnal commented Mar 25, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@picnixz picnixz changed the title concurrent.futures.Executor.map: test no reference cycle from failed future captured in its exception’s traceback gh-74028: concurrent.futures.Executor.map: test no reference cycle from failed future captured in its exception’s traceback Mar 30, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Mar 30, 2025
Make Executor.map work with infinite/large inputs correctly #74028
Closed
@picnixz picnixz added the skip news label Mar 30, 2025
@picnixz
picnixz requested a review from graingert March 30, 2025 11:36
graingert
graingert reviewed Mar 30, 2025
View reviewed changes
Comment thread Lib/test/test_concurrent_futures/executor.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@graingert

graingert commented Mar 30, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

@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 reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

graingert
graingert reviewed Mar 30, 2025
View reviewed changes
Comment thread Lib/concurrent/futures/interpreter.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@ebonnal ebonnal changed the title gh-74028: concurrent.futures.Executor.map: test no reference cycle from failed future captured in its exception’s traceback gh-74028: concurrent.futures.Executor.map: test there are no reference cycles when an exception is raised Mar 30, 2025
@ebonnal

ebonnal commented Mar 30, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

Thanks @graingert !

probably we also need to finally: del exc_wrapper here too

Nice, I have cleaned up this exc_wrapper and added an additional test that goes through the error's traceback frames and search for any reference to an Exception.

(I have renamed the PR to show that we have extended its scope)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ebonnal ebonnal changed the title gh-74028: concurrent.futures.Executor.map: test there are no reference cycles when an exception is raised gh-74028: concurrent.futures.Executor.map: test there are no reference cycles after an exception is raised Mar 30, 2025
@ebonnal ebonnal changed the title gh-74028: concurrent.futures.Executor.map: test there are no reference cycles after an exception is raised gh-74028: concurrent.futures.Executor.map: ensure there are no reference cycles after an exception is raised Mar 30, 2025
@ebonnal ebonnal changed the title gh-74028: concurrent.futures.Executor.map: ensure there are no reference cycles after an exception is raised gh-74028: concurrent.futures.Executor.map: avoid reference cycles after an exception is raised Mar 30, 2025
@ebonnal ebonnal changed the title gh-74028: concurrent.futures.Executor.map: avoid reference cycles after an exception is raised gh-74028: concurrent.futures.Executor.map: avoid reference cycles when an exception is raised Mar 30, 2025
graingert
graingert reviewed Mar 31, 2025
View reviewed changes
Comment thread Lib/concurrent/futures/interpreter.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@ebonnal ebonnal changed the title gh-74028: concurrent.futures.Executor.map: avoid reference cycles when an exception is raised gh-74028: concurrent.futures.Executor: avoid reference cycles when an exception is raised Mar 31, 2025
@ebonnal
ebonnal requested a review from graingert March 31, 2025 20:34
@ebonnal

ebonnal commented Mar 31, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@graingert via my recent cosmetic changes I have noticed that the get_referrers test fails from time to time in a non deterministic way (e.g. https://github.com/python/cpython/actions/runs/14169734692/job/39690706469?pr=131701#step:22:777):

  • build: it only concerns the free-threading builds (configured with --disable-gil)
  • test: the failing module is either test_interpreter_pool or test_process_pool
  • platform: only on Ubuntu or Windows, the test never fails on macos. (I confirm that I have successfully reproduced the issue multiple times on an Ubuntu VM, and never on my local macos).

-> as a deterministic workaround in those situations, I have added an explicit gc collection before the test:

if (
    '--disable-gil' in get_config_var("CONFIG_ARGS")
    and sys.platform in ("linux", "win32")
):
    gc.collect()

I wonder if this GC issue with free-threading builds is a known issue. I also wonder if we should investigate this further as part of this PR or if we should open an issue 🤔 Let me know your thoughts!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@graingert

graingert commented Apr 1, 2025

Copy link
Copy Markdown
Contributor

When debugging these sorts of things I usually use a mix of the packages objgraph and refcycle. Can you repeat this locally? I'll see if I can find out what's going on

ebonnal reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@graingert
graingert marked this pull request as draft April 3, 2025 07:11
@bedevere-app bedevere-app Bot removed the awaiting review label Apr 3, 2025
@graingert

graingert commented Apr 3, 2025

Copy link
Copy Markdown
Contributor

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

ebonnal reacted with laugh emoji
All reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ebonnal

ebonnal commented Apr 5, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Hey @graingert, update from my investigations with a free-threading build on an Ubuntu ARM machine:
It fails like 50% of the time, looking like some GC race condition, but the test is always successful if we pause for a bit with a time.sleep(1) before checking the referrers.
Maybe for the scope of this PR we should go back to the state of 03f8ab4 and just add this little pause?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@graingert

graingert commented Apr 7, 2025

Copy link
Copy Markdown
Contributor

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 reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ebonnal
ebonnal marked this pull request as ready for review April 7, 2025 12:45
ebonnal and others added 18 commits October 20, 2026 23:11
@ebonnal @ambv
call gc.collect for free-threading on linux/windows
ed0e87f
@ebonnal @ambv
Revert "call gc.collect for free-threading on linux/windows"
b3f3f4a
This reverts commit cae6474.
@ebonnal @ambv
Revert "skip referrers test for free-threading build on win/linux"
e3e75ce
This reverts commit fc7a569.
@graingert @ambv
more gc referrers
70e22a1
@graingert @ambv
even more gc referrers
12abc0e
@graingert @ambv
delete a ref to work_item
cac4aac
@graingert @ambv
delete reference to result_item before joining process
66b98b4
@graingert @ambv
unpack ResultItem and WorkItem
68fcecb
@graingert @ambv
fix typo
0be8f49
@graingert @ambv
bit of a hack?
6c2406d
@ebonnal @ambv
Revert investigations
7037c4a
@ebonnal @ambv
pause before checking referrers
0568262
@ebonnal @ambv
add comment about the avoided ref cycle with exc_wrapper
cfeaa9b
@ebonnal @ambv
format test
346b4ea
@ebonnal @ambv
make the self-captured exceptions check not rely on var name
2a25b9a
@ebonnal @ambv
use traceback.walk_tb
fd9b9bb
@ebonnal @ambv
comment the skipping of the current frame
682a719
@ebonnal @ambv
test_map_exception: improve readability
fb0c522
@ambv
ambv force-pushed the test/executor-map-ref-cycles branch from 82542f3 to fb0c522 Compare October 20, 2026 21:17
@ambv
ambv requested a review from ericsnowcurrently as a code owner October 20, 2026 21:17
@bedevere-app bedevere-app Bot added the type-feature A feature request or enhancement label Oct 20, 2026
@ambv
Fix CI: break exception ref cycle in _MapResultIterator.__next__ (pyt…
e0a031d
…hongh-74028)

Hold exc across 'raise exc' keeps the frame in the exception's referrers via
its traceback; clear it in a finally like the interpreter.py fix so the
 strengthened test_map_exception passes for all pool types.

Co-authored-by: Muse Code (Meta Muse Spark)
@ambv
ambv force-pushed the test/executor-map-ref-cycles branch from 05ba98a to e0a031d Compare October 21, 2026 01:52
@ambv
test_map_exception: wait for transient referrers instead of sleep(1)
6c8f6d6
The thread that completed the future can still hold the exception (via the future) when the main thread wakes up. Poll with a deadline instead of sleeping a fixed second so the test stays fast normally and robust under load, especially on free-threaded builds.

Co-authored-by: Muse Code (Meta Muse Spark)
ambv
ambv approved these changes Oct 21, 2026
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Oct 21, 2026
@ambv
ambv enabled auto-merge (squash) October 21, 2026 18:42
@ambv
ambv merged commit 4869640 into python:main Oct 21, 2026
53 of 54 checks passed

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the awaiting merge label Oct 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kumaraditya303 kumaraditya303 kumaraditya303 left review comments
@ambv ambv ambv approved these changes
@graingert graingert Awaiting requested review from graingert
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news stale Stale PR or inactive for long period of time. type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@ebonnal @graingert @ambv @kumaraditya303 @picnixz
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.