Bandit returns :closed error as a string for chunk

[danschultzer]

I got a CSV export controller that in production occassionally gives a match error, because I'm expecting {:error, :closed} for Plug.Conn.chunk/2 as the example from the docs shows:

Enum.reduce_while(~w(each chunk as a word), conn, fn (chunk, conn) ->
  case Plug.Conn.chunk(conn, chunk) do
    {:ok, conn} ->
      {:cont, conn}
    {:error, :closed} ->
      {:halt, conn}
  end
end)

However, Bandit returns a string ({:error, "closed"}) on this line:

bandit/lib/bandit/adapter.ex

Line 204 in 84e0d5d

error -> {:error, Exception.message(error)}

Which ultimately is from this line for the HTTP/1 socket:

bandit/lib/bandit/http1/socket.ex

Lines 428 to 430 in 84e0d5d

	defp request_error!(reason, plug_status \\ :bad_request) do
	raise Bandit.HTTPError, message: to_string(reason), plug_status: plug_status
	end

When I tested with HTTP/2 it seems like the process just continues to completion even though I closed the connection client side? If I let it wait for a long time it just silently stops.

So I've got two questions:

1. Shouldn't the HTTP/1 adapter return the :closed atom? I think we've looked at this before with no specified return types, but the example does show that Plug.Conn.chunk/2 returns {:error, :closed} (so at least the example needs to be updated there to show both cowboy and bandit).

2. Is it correct behavior to let HTTP/2 continue to process the request even though the client cancelled the request? Maybe I was testing this wrong or I don't understand how the HTTP/2 stream is supposed to work, but I couldn't get it to stop processing the request though I cancelled the download and closed the connection in the browser.

[mtrudel]

Shouldn't the HTTP/1 adapter return the :closed atom? I think we've looked at this before with no specified return types, but the example does show that Plug.Conn.chunk/2 returns {:error, :closed} (so at least the example needs to be updated there to show both cowboy and bandit).

Yep, that was a shortcoming of how we pushed all errors through Bandit.HTTPError, which (like all exceptions) requires a binary message. I added Bandit.SocketError in #433 to better capture the semantic of socket level errors (since they're unrecoverable as well, so shouldn't be signalled back to the client).

Is it correct behavior to let HTTP/2 continue to process the request even though the client cancelled the request? Maybe I was testing this wrong or I don't understand how the HTTP/2 stream is supposed to work, but I couldn't get it to stop processing the request though I cancelled the download and closed the connection in the browser.

HTTP/2 has a much different process model; the browser will typically keep the underlying TCP session (the 'connection' in HTTP/2 lingo) open, and only close the specific stream. We should be doing this here, which should bubble up to the stream process terminating. Do you have a repro case I can look at?

[danschultzer]

Yup, here's a demo: https://github.com/danschultzer/bandit_chunk_demo (sends CSV chunks every second over 10 seconds)

I've set up https in that repo on https://localhost:4001 so you can see how HTTP/2 function with chunked response and cancelled request.

[mtrudel]

Yep, that was a bug introduced in the big HTTP/2 refactor earlier this year, where we weren't checking for any RST_STREAM messages from the client before writing responses. Fixed on this branch, along with more consistent :closed error codes for HTTP/1.

@danschultzer would you be able to see if this branch addresses your issues in the manner you'd hoped?

[danschultzer]

Yup, it works for both HTTP/2 and HTTP/1.1, thanks!

[mtrudel]

Great to hear! I'll get this out later this week

[mtrudel]

Fixed in 1.6.1, just released