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

Add Unix Domain Socket support #139

Merged
merged 6 commits into from
Aug 11, 2020
Merged

Add Unix Domain Socket support #139

merged 6 commits into from
Aug 11, 2020

Conversation

florimondmanca
Copy link
Member

@florimondmanca florimondmanca commented Aug 6, 2020

"For consideration" :-)

Closes #138

Based on work in encode/httpx#511

I think it turns out the best option is really just to add uds=... support like for local_address. UDS is just a different network transport layer than TCP, but all of HTTP still applies, and so we can reuse our connection pool, HTTPS support, HTTP/2 support, etc.

It's a bit disappointing because it'd be nice it this was easy to create as a separate transport, but a/ reusing the connection pool while customizing how the socket is established is pretty much impossible, and b/ re-implementing everything just to change a few lines in AsyncConnectionPool and AsyncHTTPConnection seems overkill (and if I had to implement this as a third-party package, I'd find it pretty daunting).

@tomchristie
Copy link
Member

Will need to take a comprehensive review over it, but yeah I'm in favour of this.

Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it! I'm happy with this whenever you are.
Let's also doc it up alongside encode/httpx#1165

I don't think we want to push these configurations into high level API as well, it's valid for us to try to keep the surface area lower there, while still providing for some more advanced stuff.

Also there's a nice consistency in that Client(transport=...) makes it more naturally clear that you're probably also disabling auto env-based proxies from being used, because it naturally implies "This is the transport your requests will be sent over". Whereas Client(uds=...) feels much more ambiguous there, leading to... "What happens if I'm using uds=... and I've got ALL_PROXY set?".

@florimondmanca florimondmanca merged commit 2328b0c into master Aug 11, 2020
@florimondmanca florimondmanca deleted the uds branch August 11, 2020 15:57
@lundberg
Copy link
Contributor

Awesome ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UDS Support
3 participants