Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some more public APIs? #106

Closed
notpushkin opened this issue Jun 5, 2020 · 6 comments
Closed

Some more public APIs? #106

notpushkin opened this issue Jun 5, 2020 · 6 comments
Labels
question Further information is requested

Comments

@notpushkin
Copy link

Hey there and thanks for your awesome libraries!

I was planning on reviving @florimondmanca's https://github.com/florimondmanca/httpx-unixsocket-poc. I've managed to get it working but hesitate publishing a proper package for now as it's tightly coupled with things that are now private APIs.

I understand that publishing a lot of APIs might not be in scope for httpcore but it is useful in some cases like this one. Are there any plans to...

  • publish httpcore._backends.asyncio.SocketStream and httpcore._async.connection.AsyncHTTPConnection?
  • allow AsyncHTTPConnection to take a custom backend as an input?

Also, publishing httpcore._exceptions and maybe httpcore._types might be a good idea as those are useful on the outside of the library (e. g. the only way to catch TimeoutException only is to import it from the private module :-)

@florimondmanca
Copy link
Member

florimondmanca commented Jun 6, 2020

For the particular case of UDS, I think this is strongly related to #88, "advanced connection options"… Don't know if there's a way we could allow passing a pre-configured socket (eg pre-bound to a Unix socket) rather than having to rebuild a whole part of the networking stack…

@florimondmanca
Copy link
Member

@notpushkin My POC there is a bit old now — we didn't have the transport API nor HTTPCore at the time. So I gave this a second try myself and arrived to this gist, an AsyncUDSTransport implemented in ~20 LOC.

The key to simplify things was to note that we don't actually need any kind of connection pooling for UDS (since there's only one connection, i.e. the one via the Unix socket).

So the implementation there is a vanilla async transport, which manages an AsyncHTTPConnection, constructing it from an AsyncSocket built from asyncio.open_unix_connection().

So the two bits of currently private HTTPCore API I needed were:

from httpcore._async.connection import AsyncHTTPConnection
from httpcore._backends.asyncio import SocketStream

And relying on the fact that AsyncHTTPConnection accepts a socket: AsyncSocketStream argument. (We added it for the HTTP proxy implementation; interestingly it turns out to be useful in this case too.)

@florimondmanca
Copy link
Member

florimondmanca commented Jun 6, 2020

To be honest, though, most of the code in AsyncHTTPConnection can be stripped out as well in the UDS case, because:

  • A lot of it is stuff related to connection pooling (eg tracking the state of connections, and new connections, etc).
  • Some bits for proxies (.start_tls()) can be dropped too.
  • We most likely don't really need HTTP/2.

So I switched to HTTP11Connection in the gist, and it feels even cleaner now… The required bits of private API are now:

from httpcore._async.http11 import AsyncHTTP11Connection
from httpcore._backends.asyncio import SocketStream

@notpushkin
Copy link
Author

Thanks for taking time to loo into this! Your implementation looks much cleaner than mine (at a cost of dropping HTTP/2 support, which of course doesn't make much sense with UDS :)

Also, publishing httpcore._exceptions and maybe httpcore._types might be a good idea as those are useful on the outside of the library (e. g. the only way to catch TimeoutException only is to import it from the private module.

Just noticed in your example that exceptions are in fact a part of the public interface so this side note isn't strictly valid (but a mention in docs would be really great! Should I send a PR sometime after I finish my current project?)

@tomchristie
Copy link
Member

tomchristie commented Jun 8, 2020

I'd be keen for us to push back on expanding the API surface area wherever possible, for as long as we can.

@florimondmanca's suggestions wrt. connection pooling and HTTP/1.1 vs HTTP/2 w/ UDS are really valid here.
Given those remarks I actually think it'd be feasible enough for someone to maintain an entirely independant package implementing a UDS transport class, since our HTTP/1.1 implementation is pretty modest in size, with h11 picking up most the slack.

A really good starting point would be for someone to thrash out a proof of concept of a UDS transport class, based on our HTTP11Connection class.

@florimondmanca
Copy link
Member

Hello,

Since exceptions are now public, and we're now tracking UDS support via #138, I'll close this…

@florimondmanca florimondmanca added the question Further information is requested label Aug 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants