Skip to content

locks: Restore permits when async context entry is cancelled - #3770

Open
widechaos wants to merge 2 commits into
tornadoweb:masterfrom
widechaos:fix-cancelled-context-acquisition
Open

widechaos wants to merge 2 commits into
tornadoweb:masterfrom
widechaos:fix-cancelled-context-acquisition

Conversation

@widechaos

Copy link
Copy Markdown

When a task waiting to enter async with is cancelled just after release() wakes it, the permit has already been assigned to its acquisition future. The task never enters the context, so __aexit__ cannot return the permit. Subsequent users of the lock or semaphore can then wait indefinitely.

I reproduced this by holding a lock, starting a waiting context, releasing it, and cancelling the task before its next event-loop turn. The fix returns the permit when context entry is cancelled after acquisition completed. Cancellation while the acquisition is still pending leaves the permit count unchanged.

The regression tests exercise both timings for Semaphore, BoundedSemaphore, and Lock, including a queued successor. All three handoff cases fail before the fix and pass afterward.

Validation on Python 3.13:

  • python -m tornado.test tornado.test.locks_test: passed.
  • Tox py3: passed (1442 tests, 79 skipped).
  • Tox lint and docs: passed.

@bdarnell

bdarnell commented Oct 7, 2026

Copy link
Copy Markdown
Member

Thanks! This is the lock equivalent of #2826, right?

@widechaos

Copy link
Copy Markdown
Author

Yes, it is the same handoff race: #2826 loses a dequeued item when cancellation arrives after the result is assigned; here it loses a lock/semaphore permit before __aenter__ returns.

I return the permit only when acquisition has completed. Cancellation while acquisition is still pending leaves the count unchanged. This PR covers async context entry; direct acquire() callers are unchanged.

@chrikrah chrikrah left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@widechaos approving at 60c45ac. The permit goes back only when waiter finished uncancelled, which is exactly the handoff case, and a random cancellation run finds no other leak.

Head 60c45ac, merge base 134305f, Python 3.13:

$ python -m tornado.test.runtests tornado.test.locks_test
Ran 42 tests in 0.582s
OK

$ python -m tornado.test.runtests tornado.test.locks_test   # locks.py from 134305f, your tests kept
FAIL: test_cancel_after_handoff (...) (primitive='Semaphore')
FAIL: test_cancel_after_handoff (...) (primitive='BoundedSemaphore')
FAIL: test_cancel_after_handoff (...) (primitive='Lock')
FAILED (failures=3)

Then 30 workers per trial doing async with, with random task.cancel() calls between event-loop turns, 500 seeds. A trial fails if a worker is still blocked after 50 ms or the counter ends below its start:

$ python fuzz.py          # 134305f
Semaphore: 111 of 500 seeds leave a worker blocked forever or a permit missing
BoundedSemaphore: 111 of 500 seeds leave a worker blocked forever or a permit missing
Lock: 88 of 500 seeds leave a worker blocked forever or a permit missing
$ python fuzz.py          # 60c45ac
Semaphore: 0 of 500 seeds leave a worker blocked forever or a permit missing
BoundedSemaphore: 0 of 500 seeds leave a worker blocked forever or a permit missing
Lock: 0 of 500 seeds leave a worker blocked forever or a permit missing

non-blocking: AsyncContextManagerCancellationTest sits below the if __name__ == "__main__": unittest.main() block, so running the file directly never defines it. python tornado/test/locks_test.py -v reports Ran 40 tests, without the two new ones. The tests belong above that block.

flake8 7.3.0 and black 26.5.1 report clean on both files. # not run: the full tox matrix

A bare await sem.acquire() cancelled after the handoff still keeps the permit at 60c45ac, as your reply to Ben says:

$ python plain.py   # Semaphore(1): hold, waiter awaits acquire(), release(), cancel the waiter
plain acquire, cancel after handoff: <tornado.locks.Semaphore object at 0x76b4d8158830 [locked]>

@bdarnell, that is the #2826 race again, and since acquire() hands back a plain Future, locks.py can't close it without changing that return type. Track it on #2826, or leave it as caller responsibility?

@widechaos

Copy link
Copy Markdown
Author

Thanks for the review. I moved the direct test entry point below the cancellation regression class. Both python -m tornado.test.locks_test and direct execution now run all 42 tests successfully. The bare await acquire() cancellation behavior remains outside this PR’s async-context-manager scope.

This branch has not been deployed

No deployments
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.

3 participants