Expose socket connection interface in connection #273

Activity

1. jborean93 commented on Feb 22, 2021

   

   jborean93

   on Feb 22, 2021

   Author

   More actions

   When thinking about this a bit more, it sounds like having an open/aopen method that is purely designed to open the connection. I'll see if I can create a PR to show you what I mean.

   👍React with 👍1Reacted by Florimond Manca
2. jborean93 commented on Feb 23, 2021

   

   jborean93

   on Feb 23, 2021

   Author

   More actions

   I've just opened a draft PR that exposes the functionality I need as a public interface #274 in what I think is a suitable implementation. Happy to hear your thoughts on the approach and whether there's anything you wish to change or another approach you recommend.
3. florimondmanca commented on Feb 23, 2021

   

   florimondmanca

   on Feb 23, 2021

   Contributor

   More actions

   One thing that hits me looking at #274 and our code is that, although AsyncHTTPConnection is defined as a subclass of AsyncHTTPTransport (it "is" a transport), we don't use it as such internally.

   See:

   httpcore/httpcore/_async/connection_pool.py

   Lines 201 to 226 in 12a25fd

   	connection: Optional[AsyncHTTPConnection] = None
   	while connection is None:
   	async with self._connection_acquiry_lock:
   	# We get-or-create a connection as an atomic operation, to ensure
   	# that HTTP/2 requests issued in close concurrency will end up
   	# on the same connection.
   	logger.trace("get_connection_from_pool=%r", origin)
   	connection = await self._get_connection_from_pool(origin)
   	if connection is None:
   	connection = self._create_connection(origin=origin)
   	logger.trace("created connection=%r", connection)
   	await self._add_to_pool(connection, timeout=timeout)
   	else:
   	logger.trace("reuse connection=%r", connection)
   	try:
   	response = await connection.arequest(
   	method, url, headers=headers, stream=stream, ext=ext
   	)
   	except NewConnectionRequired:
   	connection = None
   	except Exception: # noqa: PIE786
   	logger.trace("remove from pool connection=%r", connection)
   	await self._remove_from_pool(connection)
   	raise

   In here we do a dance to acquire an existing connection from the pool, create one if needed, send a request, and then remove the connection from the pool.

   If AsyncHTTPConnection is to be a proper transport, then we should use it like a transport, i.e. we should call into __aenter__(), arequest() and __aexit__()/aclose(). Right now we only do arequest() (and aclose() when the pool gets closed). That's a bit awkward, and worth looking into before looking into any new hooks, I think.
4. florimondmanca commented on Feb 23, 2021

   

   florimondmanca

   on Feb 23, 2021

   Contributor

   More actions

   @jborean93 About #274 and the "allow connections to open their sockets via a dedicated method" idea… Also, cc @tomchristie — happy to hear your thoughts about this?

   Circling back to my last point above, actually transports are really thought of as classes with two methods: arequest(), and aclose(). Transports aren't really designed to "open things upon creation", at least not within the transport class itself. This is close enough to Trio's "async resource" idea, which is basically just an object with an aclose() method. In Trio, these "async resources" are typically created using async context managers, like open_something(), which manage the creation of the underlying I/O resources and pass them to the class constructor. Like this…

   @asynccontextmanager
   async def open_something():
       socket = await open_socket()
       something = Something(socket)
       try:
           yield something
       finally:
           await something.aclose()

   Which is used like this:

   async with open_something() as something:
       ...

   But although our transports expose the same kind of "constructor + aclose()" style, we don't really use them with the proper semantics always.

   For example, AsyncHTTPConnection deals with its own "open socket" internally by calling it upon first arequest(). That's kind of clunky. Ideally, we'd open a socket beforehand, and pass it to AsyncHTTPConnection(socket, ...). We don't do this right now (except for tunnel proxying, in which we pass an existing socket), and I think it's a problem that begs for a small refactor.

   So, we could switch to a style in which we have AsyncConnectionPool manage the opening of sockets, and pass them down to connections. It would work, and I have a local branch that passes tests to prove it. :-) See #275

   To customize "open socket" behavior, one would have to either override AsyncConnectionPool._open_socket() (if using connection pooling), or — if a single connection transport is enough — copy that code to open a socket before passing it to AsyncHTTPConnection(socket=...).

   from httpcore._async.http import AsyncHTTPConnection
   
   async def open_custom_socket(...):
       ...
   
   socket = await open_custom_socket(...)
   conn = AsyncHTTPConnection(socket=socket, origin=...)
   
   response = await conn.arequest(...)

   @jborean93 What would be the impact of this on your use case?
5. jborean93 commented on Feb 23, 2021

   

   jborean93

   on Feb 23, 2021

   Last edited by jborean93

   Author

   More actions

   If AsyncHTTPConnection is to be a proper transport, then we should use it like a transport, i.e. we should call into aenter(), arequest() and aexit()/aclose()

   Yea there seems to be a mixture of transport behaviours but ultimately it does things right. The reason why I thought adding aopen() was because it can ensure that anything that opens a socket or some other resource for an endpoint can be controlled by both __aenter__() -> aopen() and __aexit__() -> aclose(). So if a transport uses a network resource like a socket it can open and close them as needed but if not then it just does nothing.

   We don't do this right now (except for tunnel proxying, in which we pass an existing socket), and I think it's a problem that begs for a small refactor.

   I can't really question how things work here as I'm mostly fumbling around but this is what I see with the 3 classes of transports being used by httpcore in a normal operation

   • AsyncConnectionPool
     • This is public, seems to be the only public concrete (async) AsyncHTTPTransport implementation in httpcore
     • As you've said it maintains a pool of connections and just grabs/creates a AsyncHTTPConnection as needed
     • aclose() will call aclose() on all the connections in the pool
     • For my purposes it isn't usable as I have no public way to indicate a new connection was made
   • AsyncHTTPConnection
     • This is effectively a wrapper around AsyncHTTP11Connection (and the other HTTP version specific connections)
     • It creates the socket and version specific connection using that socket on the first call to arequest()
     • aclose() will call aclose() on the HTTP version specific connection it had created
   • AsyncHTTP11Connection
     • This one handles the HTTP logic itself and the direct reading/writing to the socket
     • It is initialised with an already connected socket
     • aclose() will close the H11 state machine and the socket that it is using

   So we already do have actual instances where we pass in a socket (ignoring tunneled proxies) I was mostly just trying to mirror the close method with the relevant open functionality that a transprot may or may not implement.

   So, we could switch to a style in which we have AsyncConnectionPool manage the opening of sockets, and pass them down to connections. It would work, and I have a local branch that passes tests to prove it. :-) See #275

   Whatever route you would like to go forward I'm happy to follow, all I need is a way to open the connection and do the TLS handshake before I prepare my actual HTTP request. Right now that's impossible to do without calling some private methods, or re implementing the whole shebang. Still what you have in that PR will work for my scenario, I can manually create and connect to the socket myself, the next step is to just make the connection class public :) (totally understand why the project is not prepared to do that right now).

   What would be the impact of this on your use case?

   The method you have chosen works for my use case and I will be happy to adopt it once the underlying connection is made public. For now I've just re implemented (shamelessly copied) AsyncHTTP11Connection and have a thing transport over the top that manages the connection in a pool-less scenario https://github.com/jborean93/pypsrp/blob/asyncio/psrp/_wsman/_async.py. I'll probably copy your create the socket and open method in the parent transport and use that socket in the HTTP11 connection rather than the aopen() method I went with.

   👍React with 👍1Reacted by Florimond Manca
6. lovelydinosaur

   closed this as completedon Mar 12, 2021
7. encode

   locked and limited conversation to collaborators on Mar 12, 2021