Uh oh!
There was an error while loading. Please reload this page.
Remove shielding from cancellation process. - #927
Conversation
| async with Trace("response_closed", logger, self._request, kwargs={}): | ||
| if not self._closed: | ||
| self._closed = True | ||
| if self._connection._connection_should_close(): |
There was a problem hiding this comment.
Can the await inside the Trace.__aenter__ cause issues here? Ie can there be a cancellation with the await inside it that would cause a skip of the self._closed-checks and self._connection._connection_should_close()? (Might be a stupid question so sorry about that :D)
There was a problem hiding this comment.
yes this is a problem, It's also a problem if the trace function raises, I think you'll need to do this:
# synchronization.pyaclose_forcefully=anyio.aclose_forcefullydefclose_forcefully(sock):
sock.close()asyncdefaclose(self):
entered_cmgr=Falsetry:
asyncwithTrace("response_closed", logger, self._request, kwargs={}):
entered_cmgr=Trueifnotself._closed:
self._closed=Trueifself._connection._connection_should_close():
awaitself._connection.aclose()
exceptBaseException:
ifentered_cmgr:
raiseifnotself._closed:
self._closed=Trueifself._connection._connection_should_close():
awaitaclose_forcefully(self._connection)
raise| return False | ||
| self._state = HTTPConnectionState.CLOSED | ||
| return True |
There was a problem hiding this comment.
Why returning bool here? all references are trying to close connection, why avoid do it at here?
rattrayalex
commented
Aug 9, 2024
@tomchristie do you intend to take this over the line, or would it be helpful for someone else to try to take it over? (for context, users of |
There was a problem hiding this comment.
probably don't want this file
| async with Trace("response_closed", logger, self._request, kwargs={}): | ||
| if not self._closed: | ||
| self._closed = True | ||
| if self._connection._connection_should_close(): |
There was a problem hiding this comment.
yes this is a problem, It's also a problem if the trace function raises, I think you'll need to do this:
# synchronization.pyaclose_forcefully=anyio.aclose_forcefullydefclose_forcefully(sock):
sock.close()asyncdefaclose(self):
entered_cmgr=Falsetry:
asyncwithTrace("response_closed", logger, self._request, kwargs={}):
entered_cmgr=Trueifnotself._closed:
self._closed=Trueifself._connection._connection_should_close():
awaitself._connection.aclose()
exceptBaseException:
ifentered_cmgr:
raiseifnotself._closed:
self._closed=Trueifself._connection._connection_should_close():
awaitaclose_forcefully(self._connection)
raise| async with Trace("response_closed", logger, request) as trace: | ||
| await self._response_closed() | ||
| if self._connection_should_close(): | ||
| await self._network_stream.aclose() |
Any update here? Would contributions be welcome? |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
T-256
commented
Jun 28, 2025
Bump |
For handling async-cancellations we're currently shielding
closeoperations, in order to ensure we don't end up with a connection in an inconsistent state when cancellations are used.A better approach is to ensure that the state changes for this case are handled synchronously (so that cancellations won't propagate into those). While the network close remains unshielded async (it's okay for cancellations to propagate into those).
Reasoning about the state when cancellations occur is a bit fiddly, tho I think we can apply this same style of approach all the way through to remove the need for async cancellation-shielding. (Closing state is applied first synchronously. Network close operations are then attempted, and may propagate cancellation.)
Refs #922