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

Support ALPN over HTTP1 pools #250

Merged
merged 2 commits into from
Nov 14, 2023
Merged

Support ALPN over HTTP1 pools #250

merged 2 commits into from
Nov 14, 2023

Conversation

josevalim
Copy link
Contributor

No description provided.

@sneako
Copy link
Owner

sneako commented Nov 14, 2023

Hey @josevalim ! The pooling strategies for H1 vs H2 are quite different, which is why we didn't want to allow this originally. I'm sure you have a good reason for this change though, so I am curious to hear

@josevalim
Copy link
Contributor Author

@sneako in some cases, you don't know upfront if it is HTTP1 or HTTP2, so you would rather use ALPN. That's what happens in Req for example. The idea is that running a HTTP2 connection over a HTTP1 pool is better than running a HTTP1 connection over a HTTP1 pool. :)

My ultimate suggestion would be to rename the top level :protocol option to :protocols. If :http1 is listed, we always use the HTTP1 pool. If only :http2, then we can use the HTTP2 pool.

@josevalim
Copy link
Contributor Author

josevalim commented Nov 14, 2023

My ultimate suggestion would be to rename the top level :protocol option to :protocols. If :http1 is listed, we always use the HTTP1 pool. If only :http2, then we can use the HTTP2 pool.

If this is accepted, I fully delegate the implementation of this bit to @wojtekmach. :D

Thank you for your time @sneako!

@wojtekmach
Copy link
Contributor

I'd deprecate :protocol in favour of :protocols and keep :protocol for one minor version. I volunteer to do all the grunt work around that.

For Finch, I'd default protocols: [:http1] as that's more predictable as well as backwards compatible for anyone not explicitly setting protocol for their pool. For Req I'm gonna default protocols: [:http1, :http2] however.

@sneako
Copy link
Owner

sneako commented Nov 14, 2023

That makes sense and I'm happy to delete our MintHttp1 module. The approach @wojtekmach describes sounds good to me. I've really been slacking on cutting a new release of Finch, but I promise I'll do that as soon as the :protocol -> :protocols change is merged. Thanks guys!

@sneako sneako merged commit 768f805 into sneako:main Nov 14, 2023
2 checks passed
@josevalim josevalim deleted the jv-support-alpn branch November 14, 2023 14:16
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.

3 participants