Remove shielding from cancellation process.

[lovelydinosaur]

For handling async-cancellations we're currently shielding close operations, 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

[MarkusSintonen]

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)

[graingert]

yes this is a problem, It's also a problem if the trace function raises, I think you'll need to do this:

# synchronization.py
aclose_forcefully = anyio.aclose_forcefully
def close_forcefully(sock):
    sock.close()

    async def aclose(self):
        entered_cmgr = False
        try:
            async with Trace("response_closed", logger, self._request, kwargs={}):
                entered_cmgr = True
                if not self._closed:
                    self._closed = True
                    if self._connection._connection_should_close():
                        await self._connection.aclose()
        except BaseException:
            if entered_cmgr:
                raise
            if not self._closed:
                self._closed = True
                if self._connection._connection_should_close():
                    await aclose_forcefully(self._connection)
            raise

[T-256]

There are also in connection_pool and http2

[T-256]

Why returning bool here? all references are trying to close connection, why avoid do it at here?

[T-256]

junk?

[rattrayalex]

@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 openai-python are reporting unusably slow performance, which seems related to encode/httpx#3215)

[graingert]

probably don't want this file

[graingert]

yes this is a problem, It's also a problem if the trace function raises, I think you'll need to do this:

# synchronization.py
aclose_forcefully = anyio.aclose_forcefully
def close_forcefully(sock):
    sock.close()

    async def aclose(self):
        entered_cmgr = False
        try:
            async with Trace("response_closed", logger, self._request, kwargs={}):
                entered_cmgr = True
                if not self._closed:
                    self._closed = True
                    if self._connection._connection_should_close():
                        await self._connection.aclose()
        except BaseException:
            if entered_cmgr:
                raise
            if not self._closed:
                self._closed = True
                if self._connection._connection_should_close():
                    await aclose_forcefully(self._connection)
            raise

[graingert]

this isn't tracing anymore

[rattrayalex]

Any update here? Would contributions be welcome?

[stale]

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]

Bump