Confusing `Mint.HTTP2.open?/2`

[mruoss]

Hey there

Sorry but this comes as a bit of a story. ;) I'm coming over from a bug that was found on k8s. Trying to solve it I found out that I am - or was? - somewhat confused about how to get the correct connection state using Mint.HTTP.open?/2

Your documentation suggests in multiple places that not Mint.HTTP.open(conn) means the connection is closed.

From Mint.HTTP.close/0:

Once the socket is closed, the connection goes into the "closed" state and open?/1 returns false.

Or Mint.HTTP.open? /2:

The type argument can be used to tell whether the connection is closed only for reading, only for writing, or for both.

Especially that last bit describing the type argument misled me to believe that if Mint.HTTP.open(conn) (with the default value for type being :read_write) returns false, the connection is actually closed. But for HTTP2 connections, that is not necessarily true, is it? The connection might still be open for reading at that point.

Also from Mint.HTTP.open? /2:

If a connection is not open for reading and writing, it has become useless and you should get rid of it. If you still need a connection to the server, start a new connection with connect/4.

Well... That's a question of where our mind puts the parentheses, isn't it? 😜

• If a connection is not open for (reading and writing), it has become useless... => TRUE!
• If a connection is not (open for reading and writing), it has become useless... => FALSE! It might still be open for reading and therefore useful.

Besides that, I think what is also confusing is that type can take one of three values (:read | :write | :read_write). For someone not particularly familiar with the underlying Protocol this suggests that a connection can be open just for :writing and it made me think a lot which case I need to check when in fact - if I understand the code correctly - Mint.HTTP.open?(conn, :write) and Mint.HTTP.open?(conn, :read_write) always return the same value. So I only need to decide whether I need to check for :read or :read_write.

Okay enough now, I hope I was able share my confusion... 😄

Proposals

• Enhancing the documentation with examples on how to check for closed connections
• Maybe Mint.HTTP.close(conn, type) where type is :read_write | :write or even something like fully_closed | :closed_for_writing_only would be nice for checking whether a connection is closed.
• Alternatively, moving the "tri-state logic" to the return value would also be a possibility. I.e. a function like Mint.HTTP.connection_state/1 which returns :open, :open_for_reading or :closed.

So thanks for reading and I hope this helps. :) I'd also offer to work on a PR if you agree and like any of the proposals...

Michael

[whatyouhide]

Hi @mruoss! 👋 Thanks for the detailed report.

I’m looking over the code and yeah, open?/2 essentially only cares about type == :read or type != :read, it doesn't make a distinction between :read and :read_write.

  def open?(%__MODULE__{state: state} = _conn, type \\ :read_write)
      when type in [:read, :write, :read_write] do
    case state do
      :handshaking -> true
      :open -> true
      {:goaway, _error_code, _debug_data} -> type == :read
      :closed -> false
    end
  end

So, I agree, the docs are hard to understand. :read_write is essentially useless. A connection cannot be open for writing but not for reading, IIRC, because if the server closes the reading side then we (Mint) close the writing side. So the only case in which open?/2 will return true for a half-closed connection is when the connection is open for reading (the server can send us data) but closed for writing (the server told us to stop writing). In that case, open?(conn, :write | :read_write) == false, while open?(conn, :read) == true.

I think we could potentially get rid of :read_write, that is, deprecate it. Thoughts @ericmj?

Either way, improvements to the docs are very welcome. 🙃

[mruoss]

Hey Andrea

Thanks for your reply. Thought a bit more about it and I guess what bothers me most is the default value for type. With this default value, open?(conn) returns false although the connection can technically still be open (for reading this is, if it is). It is also the reason not open?(conn) is not equivalent to the connection being closed. That being said, I guess there's not much we can do about that. Changing the default value would be one nasty breaking change.

Deprecating one of the options would probably help. However, I'd vote for deprecating :write rather than :read_write as then the function would read "open for read" or "open for read and write". As neither :write nor :read_write are actually used in the the function body, it is a question whether they'd have to be deprecated (versus just removing them). But I guess breaking the dialyzer would be problematic, too.

Either way, improvements to the docs are very welcome. 🙃

Okay, I'll see what I can do. 😉

[ericmj]

Is there anything more to do on this issue? I think we can deprecate :read_write but I have no strong opinion.

[whatyouhide]

Yeah I'd deprecate :read_write at this point @ericmj?