Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bandit returns :closed error as a string for chunk #432

Open
danschultzer opened this issue Nov 26, 2024 · 2 comments · May be fixed by #433
Open

Bandit returns :closed error as a string for chunk #432

danschultzer opened this issue Nov 26, 2024 · 2 comments · May be fixed by #433

Comments

@danschultzer
Copy link
Contributor

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 mtrudel linked a pull request Nov 29, 2024 that will close this issue
Open
@mtrudel
Copy link
Owner

mtrudel commented Nov 29, 2024

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
Copy link
Contributor Author

danschultzer commented Nov 29, 2024 •
edited
Loading

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Improve unrecoverable error handling mtrudel/bandit
2 participants
@mtrudel @danschultzer