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

Should mint be responsible for error handling empty address? #351

Closed sato11 closed 2 years ago

sato11 commented 2 years ago

Taking a look into https://github.com/sneako/finch/issues/186, I've found the case where :gen_tcp.connect/4 throws :badarg which mint is unable to handle. Is it in mint's scope to take care of it? Personally I'd find it great if it was since I feels that zero byte strings are not inconsistent with Mint.Types.address() which contains String.t(). What do you think?

iex(31)> Mint.HTTP.connect(:http, "httpbin.org", 80)  
{:ok,
 %Mint.HTTP1{
   buffer: "",
   host: "httpbin.org",
   mode: :active,
   port: 80,
   private: %{},
   proxy_headers: [],
   request: nil,
   requests: {[], []},
   scheme_as_string: "http",
   socket: #Port<0.9>,
   state: :open,
   streaming_request: nil,
   transport: Mint.Core.Transport.TCP
 }}
iex(32)> Mint.HTTP.connect(:http, "httpbin.void", 80)
{:error, %Mint.TransportError{reason: :nxdomain}}
iex(33)> Mint.HTTP.connect(:http, "", 80)            
** (exit) :badarg
    (kernel 8.3.1) gen_tcp.erl:218: :gen_tcp.connect/4
    (mint 1.4.0) lib/mint/core/transport/tcp.ex:41: Mint.Core.Transport.TCP.connect/3
    (mint 1.4.0) lib/mint/http1.ex:115: Mint.HTTP1.connect/4
wojtekmach commented 2 years ago

I believe the problem is not that Mint crashes for this value but that the reason is very opaque. A common solution is to add additional information in accordance with https://www.erlang.org/eeps/eep-0054. Given it is an exit we cannot do that though. So unfortunately I don’t think Mint can do much about it. (It could special case "" but eg "/(*" would be invalid too.) I think the real fix would have to happen in OTP.

sato11 commented 2 years ago

Hi, thanks for the feedback and the link. I had a feeling that this is maybe OTP issue, but I was not very sure about it and asked here beforehand. But thanks to your comment I guess it's making sense to me. I'll be reporting it in OTP after a little more research. Thanks a lot!