Closed the-mikedavis closed 2 years ago
@the-mikedavis Thanks for opening the PR. It looks like you are on the right path, let us know if you need any help or it's ready for review.
Ok so I ran into some trouble with the test case. Initially I tried changing this block https://github.com/elixir-mint/mint/blob/cf3ecabd16dc5a461e7e88919bef6508d553c0d5/test/support/mint/http2/test_server.ex#L97-L108 to close the socket for writing by adding in a clause for goaway frames
goaway(error_code: :no_error) = frame, server ->
:ok = :ssl.shutdown(server.socket, :write)
{Frame.encode(frame), server}
to emulate the close-for-writing behavior, so that the test would fail when run on main
. That worked most of the time but on some runs of mix test
, the call to set the socket as active-once https://github.com/elixir-mint/mint/blob/cf3ecabd16dc5a461e7e88919bef6508d553c0d5/lib/mint/http2.ex#L798-L801
would fail with {:error, %Mint.TransportError{reason: :closed}}
which would mess up the test case a bit.
So instead I settled for asserting that the client sends no frames to the server when handling the data frame after the goaway, which reliably passes on this branch and fails on main
.
I'm definitely open to advice if there's a nicer way of testing this that I'm missing! Other than that I think this PR is ready to go :+1:
I love this, great catch @the-mikedavis! 💟
I found an odd scenario where a connection that is closed for writing will fail to
Mint.HTTP2.stream/2
any DATA frames that come in after the connection becomes read-only.I can attach the pcap if you'd like but since all that takes a bunch of work I've added a screenshot of the wireshark view of the scenario I found that triggers the behavior:
In this case a request opens up and sends some data. Then the server sends a GOAWAY (no error) that closes the connection for writing, and then it finishes out the request by sending the response HEADERS and a DATA frame (with the end stream flag). Mint parses that HEADERS into the appropriate
[{:headers, ref, headers}, {:status, ref, status}]
responses, but when it gets to handling the DATA frame (handle_data/3
) it gets into some trouble callingsend!/2
.It's using
send!/2
by callingrefill_client_windows/3
andclose_stream!/3
, which ends up in one of these throws: https://github.com/elixir-mint/mint/blob/cf3ecabd16dc5a461e7e88919bef6508d553c0d5/lib/mint/http2.ex#L2047-L2051So ultimately in this case
Mint.HTTP2.stream/2
returnsBut this PR changes the return for this case to
I'm starting with this PR as a draft as I try to figure out how to test this case :sweat_smile: