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

Do not send a fallback if the plug has already sent a response #289

Merged
merged 2 commits into from
Jan 12, 2024
Merged

Do not send a fallback if the plug has already sent a response #289

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf46843
Do not send a fallback if the plug has already sent a response
mtrudel Jan 11, 2024
62fcbd4
Update test/bandit/http1/request_test.exs
mtrudel Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions lib/bandit/http1/handler.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ defmodule Bandit.HTTP1.Handler do

use ThousandIsland.Handler

@already_sent {:plug_conn, :sent}

@impl ThousandIsland.Handler
def handle_data(data, socket, state) do
connection_span = ThousandIsland.Socket.telemetry_span(socket)
Expand Down Expand Up @@ -109,9 +111,17 @@ defmodule Bandit.HTTP1.Handler do
defp code_for_reason(_), do: 400

defp attempt_to_send_fallback(req, code) do
Bandit.HTTP1.Adapter.send_resp(req, code, [], <<>>)
rescue
_ -> :ok
receive do
@already_sent ->
send(self(), @already_sent)
after
0 ->
try do
Bandit.HTTP1.Adapter.send_resp(req, code, [], <<>>)
rescue
_ -> :ok
end
end
Comment on lines +114 to +124
Copy link
Contributor

@ryanwinchester ryanwinchester Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reading it, it seems like such an obvious solution but I never would have though of it 🤔 💡 👍

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cribbed directly from plug/error_handler.ex source: https://github.com/elixir-plug/plug/blob/v1.15.2/lib/plug/error_handler.ex#L103

end

defp maybe_keepalive(req, state) do
Expand Down
14 changes: 14 additions & 0 deletions test/bandit/http1/request_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1393,6 +1393,20 @@ defmodule HTTP1RequestTest do
raise "boom"
end

@tag capture_log: true
test "does not send an error resposne if the plug has already sent one before raising",
mtrudel marked this conversation as resolved.
Show resolved Hide resolved
context do
client = SimpleHTTP1Client.tcp_client(context)
SimpleHTTP1Client.send(client, "GET", "/send_and_raise_error", ["host: banana"])
assert {:ok, "200 OK", _headers, _} = SimpleHTTP1Client.recv_reply(client)
assert SimpleHTTP1Client.connection_closed_for_reading?(client)
end

def send_and_raise_error(conn) do
send_resp(conn, 200, "OK")
raise "boom"
end

@tag capture_log: true
test "returns a 500 if the plug does not return anything", context do
response = Req.get!(context.req, url: "/noop")
Expand Down