Skip to content

Avoid cancellation checkpoint after temporary directory creation - #1403

Merged
agronholm merged 2 commits into
agronholm:masterfrom
adenzhou1350:codex/tempdir-cancel-owner-check
Oct 10, 2026
Merged

agronholm merged 2 commits into
agronholm:masterfrom
adenzhou1350:codex/tempdir-cancel-owner-check

Conversation

@adenzhou1350

@adenzhou1350 adenzhou1350 commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

NOTE Erasing or replacing the contents of this template will result in your pull
request being summarily closed without consideration!

Changes

No linked issue.

Avoid a cancellation checkpoint between creating a temporary directory and returning from TemporaryDirectory.__aenter__. If cancellation arrives during creation, the second worker call can raise before the async with body starts, so __aexit__ never runs and a retained manager keeps the directory until explicit cleanup or finalization.

Return the created directory's name directly, matching stdlib TemporaryDirectory.__enter__, which only returns that attribute. Pending cancellation then reaches the body's checkpoint and the existing shielded __aexit__ removes the directory. The regression cancels from the creation worker and keeps the manager alive.

Validation on native CPython 3.12.13: the regression fails on all four backend variants on both Windows and Linux without the fix; the fixed tempfile module passes 84 tests on each platform (asyncio, winloop/uvloop, eager asyncio, Trio). Changed-file Ruff 0.16.10 checks and formatting pass on Linux. Optional Blockbuster instrumentation and the full suite were not run.

This fixes acquisition, not the explicit cleanup() cancellation case covered by #1316. No API or documented usage changes.

Checklist

If this is a user-facing code change, like a bugfix or a new feature, please ensure that
you've fulfilled the following conditions (where applicable):

  • You've added tests (in tests/) which would fail without your patch
  • You've updated the documentation (in docs/), in case of behavior changes or new
    features
  • You've added a changelog fragment (in changelog.d/).

If this is a trivial change, like a typo fix or a code reformatting, then you can ignore
these instructions.

Updating the changelog

Add a news fragment file in the changelog.d/ directory, named
<issue or PR number>.<type>.rst, where <type> is one of:

  • breaking: backwards incompatible changes
  • added: new features
  • changed: other user-visible changes
  • fixed: bug fixes

If, say, your patch fixes issue #123, create changelog.d/123.fixed.rst
containing only a brief summary of the change, like this:

Fixed big bad boo-boo in task groups

The issue link and the credit for your pull request are added automatically on release.
If there's no issue linked, name the file after your pull request instead by adding the
fragment after you've created the PR.

Signed-off-by: Xucheng Zhou <aden1350@outlook.com>
Signed-off-by: Xucheng Zhou <aden1350@outlook.com>
@agronholm

Copy link
Copy Markdown
Owner

Returning the name directly without to_thread.run_sync() seems sensible, but wouldn't the same issue occur with an already-cancelled scope when entering the async context manager? It would create the directory and then react to cancellation afterwards.

@adenzhou1350

Copy link
Copy Markdown
Contributor Author

I checked this on asyncio and Trio (Python 3.12): with an already-cancelled scope, the initial to_thread.run_sync() checkpoint raises before calling the constructor (zero calls), so no directory is created.

The regression covers cancellation while the constructor is running. With the default abandon_on_cancel=False, that call returns the directory despite pending cancellation. Returning .name then lets async with enter its body; cancellation at the next checkpoint runs the shielded __aexit__. The former second run_sync(__enter__) instead raised before the body was entered, leaving cleanup dependent on the retained manager's lifetime.

@agronholm agronholm left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@agronholm
agronholm merged commit fc141a7 into agronholm:master Oct 10, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants