search

elixir-mint / mint

Functional HTTP client for Elixir with support for HTTP/1 and HTTP/2 🌱
Apache License 2.0
1.36k stars 112 forks source link

Drop https options from tcp connection #315

Closed ono closed 3 years ago

ono commented 3 years ago

You might say this is not a right place to put this logic but it can be handy if Mint applies these options only for https.

We use Mint through Finch/Tesla and connection option is given at the start of the application (Finch.start_link/1). We would like to put TLS options (cacertfile) there but the option will be applied regardless the request scheme. It's fine for https but :gen_tcp.connect/4 would raise :badarg error for these options.

The change ensures these options are dropped when you make non https requests so no error will be raised.

You might argue that I should put these changes on Finch layer (or workaround by having different Finch GenServer for http and https) but I would like to hear what you think.

sneako commented 3 years ago

Finch will always start a separate connection pool for every {scheme, host, port}, so I think we could filter out these options before we start an :https pool in there. Perhaps only in the case where the :default pool configuration is being used, because we can be certain that it was not intentional in that case. I’m not sure if there is any reason that someone would intentionally set these for non :https however, in which case doing it for all :https pools could be fine.

ono commented 3 years ago

@sneako sounds good. i will open a PR on Finch shortly. thank you for your quick answer!