Repository navigation
Conversation
|
Thanks! This is the lock equivalent of #2826, right? |
|
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 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 |
chrikrah
left a comment
There was a problem hiding this comment.
@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?
|
Thanks for the review. I moved the direct test entry point below the cancellation regression class. Both |
When a task waiting to enter
async withis cancelled just afterrelease()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, andLock, 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.py3: passed (1442 tests, 79 skipped).lintanddocs: passed.