search

sneako / finch

Elixir HTTP client, focused on performance
MIT License
1.23k stars 114 forks source link

Support ALPN over HTTP1 pools #250

Closed josevalim closed 7 months ago

sneako commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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!