HTTP/2: respect closed-for-writing when streaming data frames

[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 calling send!/2.

It's using send!/2 by calling refill_client_windows/3 and close_stream!/3, which ends up in one of these throws:

mint/lib/mint/http2.ex

Lines 2047 to 2051 in cf3ecab

	{:error, %TransportError{reason: :closed} = error} ->
	throw({:mint, %{conn | state: :closed}, error})
	{:error, reason} ->
	throw({:mint, conn, reason})

So ultimately in this case Mint.HTTP2.stream/2 returns

{:error, conn, %Mint.TransportError{reason: :closed}, [{:status, ref, status}, {:headers, ref, headers}]}

But this PR changes the return for this case to

{:ok, conn, [{:status, ref, status}, {:headers, ref, headers}, {:data, ref, data}, {:done, ref}]}

I'm starting with this PR as a draft as I try to figure out how to test this case 😅

[ericmj]

@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.

[the-mikedavis]

Ok so I ran into some trouble with the test case. Initially I tried changing this block

mint/test/support/mint/http2/test_server.ex

Lines 97 to 108 in cf3ecab

	{data, server} =
	Enum.map_reduce(frames, server, fn
	{frame_type, stream_id, headers, flags}, server
	when frame_type in [:headers, :push_promise] ->
	{server, hbf} = encode_headers(server, headers)
	flags = Frame.set_flags(frame_type, flags)
	frame = headers(stream_id: stream_id, hbf: hbf, flags: flags)
	{Frame.encode(frame), server}
	frame, server ->
	{Frame.encode(frame), server}
	end)

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

mint/lib/mint/http2.ex

Lines 798 to 801 in cf3ecab

	case transport.setopts(socket, active: :once) do
	:ok -> {:ok, conn, responses}
	{:error, reason} -> {:error, put_in(conn.state, :closed), reason, responses}
	end

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 👍

[whatyouhide]

I love this, great catch @the-mikedavis! 💟