Transport implementations: embracing __aenter__ and __aexit__

[florimondmanca]

Another thing I noticed while reworking encode/httpx#998 and thinking about encode/httpx#1145... (Actually, anyone that tries to solve encode/httpx#1145 should arrive at the same conclusion than I'm exposing in this issue, since the HTTPX clients does the wrong thing there as well, which causes issues with exception handling.)

Right now when someone wants to implement a custom transport, we tell them:

• Implement .request(...) -> ....
• Optionally implement .aclose() -> None.

I think we might want to consider switch to:

• Implement .request(...) -> ....
• Optionally implement .__aenter__() -> None and __aexit__(...) -> None.

That is, making it clear to transport implementers that transports are just context managers. More specifically, that if they want their transport to wrap around an async resource, they can do so by forwarding __aenter__() and __aexit__() to that resource.

We can probably (?) keep .aclose() as a synonym of .__aexit__(None, None, None), but it's actually not clear to me whether keeping it wouldn't maintain some sort of confusion. If we really want to enforce context managed usage of transports (and it looks like we should), then transport = Transport() (plain instantiation) + calling .aclose() is not a correct option.

So, AsyncHTTPTransport would become this:

class AsyncHTTPTransport:
    """
    The base interface for sending HTTP requests.

    Concete implementations should subclass this class, and implement
    the `request` method, and optionally the `__aenter__` and/or `__aexit__` methods.
    """

    def request(
        self,
        method: bytes,
        url: URL,
        headers: Headers = None,
        stream: AsyncByteStream = None,
        timeout: TimeoutDict = None,
    ) -> AsyncContextManager[
        Tuple[bytes, int, bytes, List[Tuple[bytes, bytes]], AsyncByteStream]
    ]:
        """
        The interface for sending a single HTTP request, and returning a response.

        **Parameters:**

        * **method** - `bytes` - The HTTP method, such as `b'GET'`.
        * **url** - `Tuple[bytes, bytes, Optional[int], bytes]` - The URL as a 4-tuple of
        (scheme, host, port, path).
        * **headers** - `Optional[List[Tuple[bytes, bytes]]]` - Any HTTP headers
        to send with the request.
        * **stream** - `Optional[AsyncByteStream]` - The body of the HTTP request.
        * **timeout** - `Optional[Dict[str, Optional[float]]]` - A dictionary of
        timeout values for I/O operations. Supported keys are "pool" for acquiring a
        connection from the connection pool, "read" for reading from the connection,
        "write" for writing to the connection and "connect" for opening the connection.
        Values are floating point seconds.

        ** Returns:**

        An asynchronous context manager returning a five-tuple of:

        * **http_version** - `bytes` - The HTTP version used by the server,
        such as `b'HTTP/1.1'`.
        * **status_code** - `int` - The HTTP status code, such as `200`.
        * **reason_phrase** - `bytes` - Any HTTP reason phrase, such as `b'OK'`.
        * **headers** - `List[Tuple[bytes, bytes]]` - Any HTTP headers included
        on the response.
        * **stream** - `AsyncByteStream` - The body of the HTTP response.
        """
        raise NotImplementedError()  # pragma: nocover

    async def __aenter__(self) -> "AsyncHTTPTransport":
        """
        Context manager hook implementation.

        I/O and concurrency resources managed by this transport should be opened here.
        """
        return

    async def __aexit__(
        self,
        exc_type: Type[BaseException] = None,
        exc_value: BaseException = None,
        traceback: TracebackType = None,
    ) -> None:
        """
        Close the implementation, which should close any outstanding I/O and concurrency
        resources managed by this transport.
        """

    async def aclose(self) -> None:
        await self.__aexit__(None, None, None)

[tomchristie]

So, I'd also been thinking about the interaction between .__exit__() and .close() / .__aexit__() and .aclose(), and planning to finish thrashing that out in encode/httpx#1145.

I don't think it's particularly problematic - the base implementation of .__exit__() should just be to call .close() - which then means that you can either use the client/transport in a context-managed style, or not. It's feasible that some transports might introduce a "you have to use this in a context managed block", for example, the ASGITransport requires a background task if we want it to support streaming responses. Our other implementations don't currently require that.

We could just enforce that constraint unilaterally throughout, and force users into...

async with httpx.AsyncClient(...) as client:
    ...

That's possible okay in a newish async ecosystem, where we can be bullish about allowed usage styles, but unless we're a bit fiddly it also forces the sync usage to only ever use the same style.

with httpx.Client(...) as client:
    ...

Which isn't a pattern that fits with existing request-based codebases, and just plain can't work properly with WSGI applications. You want to have a top-level global client, that's sharing the connection pool throughout, but WSGI doesn't expose any kind of "lifetime" scope that you can instantiate a with httpx.Client(...) as client: within. It's totally okay and normal to be using...

client = httpx.Client(...)

# Just use this global `client` instance throughout.

Right now our approach is to instead walk-the-fine-line, and say that clients don't impose a strict context managed style, but transports may choose to import one (Eg. ASGITransport might end up doing so) or may have some optional functionality that's only present if it is used (Eg. AsyncConnectionPool could perform in-background keep-alive cleanups when used as a context manager, or send periodic HTTP/2 pings to support long-lived idle connections.).

[florimondmanca]

It's feasible that some transports might introduce a "you have to use this in a context managed block", for example, the ASGITransport requires a background task if we want it to support streaming responses.

Sorry if I'm sounding cocky and repetitive here (since I've been saying this a few times in the past few days - if there's anything wrong with my reasoning here please let me know!), but again I don't think this is particularly relevant to the ASGITransport. For example, you can very well do the following:

# No need to enforce context managed usage,
# since this doesn't "spin up" anything.
transport = ASGITransport(app=app)

# ... But this runs an app in the background, so we
# do want to enforce context-managed usage here, which #145 would allow.
with transport.request(...) as ...:
    ...

---

This being said, I agree with your points about practicality of just being able to do client = Client(...) at the top level, and calling .close() whenever appropriate.

So as I understand we'd be trying to support both styles…?

1. Transports that accept Transport()/transport.aclose()
2. Transport that require usage like with Transport(): ...

In case 1/, this issue is mostly irrelevant. These dunder methods are not being called in practice, and .aclose() can be whatever it needs to be. However the current state of things also allows these transports to be used using context manager syntax (since both __aenter__ and __aexit__ have default implementations).

In case 2/, transport authors will probably want to define __aenter__ and __aexit__ anyway, so the fact that __aexit__ calls .aclose() by default is irrelevant.

Okay, this all makes sense.

---

Then I guess one extra question I'd have, and this is more of an HTTPX one, is what should high-level clients do in their own __enter__, __exit__ and .close() implementations, with respect to any bundled transports?

Here's what I think should happen so that the properties I described above are well-transfered to the high-level client layer…

• Client.__enter__ calls into the transports' __enter__.
• Client.__exit__ calls into the transports' __exit__.
• Client.close calls into the transports' close.

Sounds obvious when written like this, but currently this is what we have (bold = what needs to change):

• Client.__enter__ calls into the transports' __enter__.
• Client.__exit__ calls Client.close().
• Client.close calls into the transports' close.

So effectively right now we're not supporting context-manager-only transports well (since their __enter__ is never called).

I guess this is what encode/httpx#1145 is pretty much about, and there's nothing else we need to do here wrt HTTPCore and the transport API?

[tomchristie]

Yup, exactly all that. 😊

[tomchristie]

Closing as resolved via encode/httpx#1218