Skip to content

Fix BlockingPortal completion racing with Future cancellation - #1410

Merged
agronholm merged 7 commits into
agronholm:masterfrom
adenzhou1350:codex/fix-portal-future-completion-race
Oct 17, 2026
Merged

agronholm merged 7 commits into
agronholm:masterfrom
adenzhou1350:codex/fix-portal-future-completion-race

Conversation

@adenzhou1350

@adenzhou1350 adenzhou1350 commented Oct 11, 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.

A thread can cancel the Future after _call_func() checks cancelled() but before set_result() or set_exception(). The resulting InvalidStateError escapes the portal task and cancels its task group. Use set_running_or_notify_cancel() only when the callable has completed, so cancellation remains available while it runs and completion commits atomically.

The regression now follows the review suggestion: a real stdlib Future subclass calls cancel() immediately before set_result()/set_exception(), without an additional cancellation thread or sync/async parametrization. All 8 result/exception cases across the configured backends fail with InvalidStateError when the original from_thread.py is restored. With the fix, tests/test_from_thread.py passes on native Windows CPython 3.12: 222 passed, 4 skipped, 1 existing xfail. Ruff 0.16.10 check/format and git diff --check pass. Full pre-commit was attempted but blocked by Windows application control while starting a hook environment; the full suite, Linux and other Python versions were not rerun for this test-only follow-up. Earlier threaded-test results are historical. Implementation and tests are AI-assisted.

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.

@adenzhou1350

Copy link
Copy Markdown
Contributor Author

@agronholm Could you take a look at this portal Future completion/cancellation fix when convenient? All 20 hosted checks pass. The regression forces the completion race with real stdlib Futures and a second thread; the recorded before/after results cover synchronous/asynchronous callables and results/exceptions. The current blocker is maintainer review; GitHub reports that the branch can be cleanly merged.

Comment thread tests/test_from_thread.py Outdated
Signed-off-by: Xucheng Zhou <209601943+adenzhou1350@users.noreply.github.com>
@adenzhou1350
adenzhou1350 force-pushed the codex/fix-portal-future-completion-race branch from 8ded60f to e8f9d6a Compare October 15, 2026 06:50
Comment thread tests/test_from_thread.py Outdated

@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 633965b into agronholm:master Oct 17, 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