Fix aclose_forcefully() returning before the socket is released - #1278
Closed
Sohel2309 wants to merge 1 commit into
Closed
Fix aclose_forcefully() returning before the socket is released#1278Sohel2309 wants to merge 1 commit into
Sohel2309 wants to merge 1 commit into
Conversation
… asyncio anyio.aclose_forcefully() wraps resource.aclose() in an already-cancelled CancelScope so that closing happens as quickly as possible. On the asyncio backend, SocketStream.aclose() (used for TCP streams) relied on a single 'await sleep(0)' checkpoint between transport.close() and the forceful transport.abort() call to give the transport a chance to flush. Because aclose_forcefully() pre-cancels its scope, that checkpoint could raise a Cancelled exception (silently swallowed by the scope), which caused transport.abort() to be skipped entirely, leaving the underlying socket fd open after aclose_forcefully() returned. Separately, concurrent calls to aclose() on the same SocketStream could race: the first caller's synchronous transport.close() call already marks the transport as 'closing', so a second, concurrently running caller could see is_closing() == True and return immediately without ever performing (or waiting for) the actual abort(), again leaving the fd open at the time it returned. Fix: * Wrap the close()/checkpoint sequence in try/finally so transport.abort() always executes, even if the checkpoint is cancelled. * Track the close operation with an asyncio.Event. Once a task has claimed the role of performing the actual close, any other concurrent caller waits (shielded from its own forced cancellation) for that Event instead of taking a is_closing()-based shortcut, guaranteeing it only returns after the socket has actually been released. Added a regression test, ported from the issue report, that spawns two concurrent aclose_forcefully() calls on the same TCP stream and asserts that the raw socket fd is released (-1) for both by the time they return. It fails on current main and passes with this fix, across the asyncio, asyncio+uvloop, asyncio+eager, and trio backends. Fixes agronholm#1273.
Owner
|
Closing in favor of #1277 which is more comprehensive. Also, you deleted the PR template which you were warned about. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Fixes a race condition in
SocketStream.aclose_forcefully()on the asyncio backend where the method could return before the underlying socket had actually been released.Bug
aclose_forcefully()runsresource.aclose()inside an already-cancelledCancelScopeso that the close operation can proceed during cancellation.For asyncio
SocketStream, the close sequence relied on anawait sleep(0)checkpoint betweentransport.close()andtransport.abort().Because
aclose_forcefully()pre-cancelled its scope, that checkpoint could be cancelled andtransport.abort()could be skipped. This allowedaclose_forcefully()to return while the underlying socket file descriptor was still open.There was also a related race when multiple tasks called
aclose()concurrently: a later caller could observeis_closing()and return before the task performing the actual transport abort had completed.Fix
transport.abort()is reached even when the close checkpoint is cancelled by wrapping the close/checkpoint sequence intry/finally.asyncio.Event.is_closing()state.This guarantees that
aclose_forcefully()does not return before the underlying socket has actually been released.Regression tests
Added regression coverage for:
aclose()calls on the same socket.The tests were verified across the supported asyncio, trio, trio-asyncio, and curio backends.
Validation
Fixes #1273