Repository navigation
Transport implementations: embracing __aenter__ and __aexit__ #146
Description
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__() -> Noneand__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)
Activity
lovelydinosaur commentedon Aug 10, 2020
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, theASGITransportrequires 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...
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.
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 awith httpx.Client(...) as client:within. It's totally okay and normal to be using...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 commentedon Aug 10, 2020
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: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…?
Transport()/transport.aclose()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.closecalls 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__callsClient.close().Client.closecalls 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?
lovelydinosaur commentedon Aug 11, 2020
Yup, exactly all that. 😊
lovelydinosaur commentedon Sep 10, 2020
Closing as resolved via encode/httpx#1218