add Mint.HTTP.module/1 to describe the conn module given a conn

[andyleclair]

@ericmj it seems like that could also be achieved by making Mint.HTTP.conn_module public, but I'm not sure about UnsafeProxy. Since it uses Mint.HTTP1 under the hood should we return Mint.HTTP1? UnsafeProxy? I don't really know what UnsafeProxy gets used for, guessing non-https proxying

[whatyouhide]

Would it be better to expose the protocol instead of the module? Something like get_protocol(Mint.HTTP.t()) :: :"http1.1" | :http2?

[whatyouhide]

I see that @ericmj suggested the module-based solution in #263. Any reason not to go with the protocol instead?

[ericmj]

In the scenario from #263 I think it's better to return the connection module since they want to check if they can call functions on a specific module.

There may be a use case to get the protocol as well but that would need a different API I think since we support HTTP 0.9, 1.0, 1.1, and 2.0. Maybe protocol_version(t()) :: {pos_integer(), pos_integer()}?

[whatyouhide]

@ericmj yes I like the protocol + version tuple. Can't folks map the protocol to the HTTP module? Wondering if we should keep the module internal, that's all :)

[ericmj]

I don't think we need to keep it internal, Mint.HTTP1 and Mint.HTTP2 are public documented modules.

We can add protocol(t()) :: {atom(), pos_integer(), pos_integer()} instead but then we also need to document that you can call Mint.HTTP1 functions with a connection struct returned from Mint.HTTP.connect() if Mint.HTTP.protocol() returns {:http, 0 | 1, _}. It feels more complicated when we can instead return the module directly.
?

[whatyouhide]

I’m worried about the proxy too here. If we return the module it's not guaranteed that you can call functions on that, is it? Because it could be a proxy conn?

[andyleclair]

For our purposes, we're looking to differentiate in order to fix a bug with transfer-encoding: chunked (see also: appcues/mojito#87). We don't need to call functions on the module, the module name was good enough

[ericmj]

Because it could be a proxy conn?

Yeah, this would break on a proxy conn.

@andyleclair Sorry for the back and forth, but could you add a protocol(t()) :: {atom(), pos_integer(), pos_integer()} function instead?

[andyleclair]

hey no problem! I'm glad to help. thank you both for your time (and for considering this PR)

[andyleclair]

From my understanding, I'd need a request to determine the full protocol from the conn, right? Obviously for HTTP2, it's just {:http, 2, 0} but to tell 0.9 from 1.0 from 1.1 we'd need a request body as the version is returned from the server

[ericmj]

From my understanding, I'd need a request to determine the full protocol from the conn, right? Obviously for HTTP2, it's just {:http, 2, 0} but to tell 0.9 from 1.0 from 1.1 we'd need a request body as the version is returned from the server

Ugh, good point.

In that case I am leaning towards returning :http1 | :http2. Alternatively return the module names, but add support for %UnsafeProxy{} in Mint.HTTP1 to handle that case. But I want to hear what @whatyouhide thinks.

[andyleclair]

Under the hood UnsafeProxy is using Mint.HTTP1 but returning Mint.HTTP1 from this function strikes me as leaking an implementation detail. I don't have enough context on what UnsafeProxy is for or does, but from reading the code, it seems like Mint.HTTP1 but with some connection proxy details

[whatyouhide]

I would just go with :http1/:http2. We can call the function major_protocol or something like this so that we keep protocol/1 open for the future. Or even better, we have protocol(Conn.t()) :: :http1 | :http2 and in the future we can add protocol(Conn.t(), :with_minor_version) :: {...} if the need arises.

[andyleclair]

@ericmj @whatyouhide how does that look to you?

[ericmj]

Looks good but I think we should handle UnsafeProxy or did we decide not to?

[andyleclair]

@ericmj since Mint.HTTP.t() is defined as Mint.HTTP1.t() | Mint.HTTP2.t() I decided against it, but I can add it.

[whatyouhide]

Left a few suggestions, plus I’m requesting changes because we need to add tests for this 🙃

[whatyouhide]

We usually use this format:

Suggested change

	:http1 = Mint.HTTP.protocol(conn)
	Mint.HTTP.protocol(conn)
	#=> :http1

[andyleclair]

I added doctests, is that ok?

[andyleclair]

@whatyouhide it looks like using since: "1.4.0" breaks on older elixir/erlang versions

[whatyouhide]

@andyleclair you can do @doc since conditionally, something like this:

if Version.compare(System.version(), "<whatever version introduced @doc since>") in [:eq, :gt] do
  @doc since: "1.4.0"
end