Raise error on send errors for HTTP/1 responses

[mtrudel]

The Plug spec doesn't really provide for the effective surfacing of errors from send calls, mostly because Cowboy will (usually) have killed off the connection handler process when the client closes the connection. In Bandit's case, we don't actively kill off connection processes in this situation, so we need to be able to surface these cases effectively. Raising a Bandit.HTTPError will bubble through the caller all the way up to Bandit's handler pipeline, and log the error as configured. This is as good of a pattern as we're going to have, and mirrors what we do for reads against closed clients already (tests were added for that case here as well, since they were absent before).

Fixes #344

[mtrudel]

Hearing no objections, and having heard back good things on #344, I'm merging this

[CharlesOkwuagwu]

In Bandit's case, we don't actively kill off connection processes in this situation, so we need to be able to surface these cases

Please is there a reason why you keep the connection process alive when the client closes?

Can this behavior be a configurable option?

For cases where you need well behaved ServerSentEvents (SSE) killing the process is the correct behavior when the client closes.

[mtrudel]

Please is there a reason why you keep the connection process alive when the client closes?

It's a side-effect of the process model we use; we don't really have a choice. The key thing is that Bandit only uses a single process per connection, and this connection uses the active: :once mode of the underlying socket (gen_tcp/ssl). The only way to be informed of the underlying connection being closed is to either 1. Receive :tcp_closed messages, or 2. handle {:error, :closed} tuples that come back from the various socket functions. This PR actively adds support for that second option on send calls.

[CharlesOkwuagwu]

I've used the latest version of Bandit, I setup a test specifically for SSE.

THE handling process stays alive, long after the client closes.

Is this the expected behavior, for the option 2 you mentioned above?

This PR actively adds support for that second option on send calls.

In this case on the subsequent chunk/2 call for a closed client should see {:error, :closed} ... Yes?

[mtrudel]

I've used the latest version of Bandit, I setup a test specifically for SSE.

I'd love to see a repro for this! The SSE process is obviously doing something while it's sitting idle and it may be possible to have it look at the connection status while it's doing this

[mtrudel]

(Feel free to reopen this issue, btw)

[CharlesOkwuagwu]

I've used the latest version of Bandit, I setup a test specifically for SSE.

I'd love to see a repro for this! The SSE process is obviously doing something while it's sitting idle and it may be possible to have it look at the connection status while it's doing this

Sorry for the delay

https://github.com/CharlesOkwuagwu/sse_demo

[mtrudel]

Which client are you using @CharlesOkwuagwu ? I've forked your repo locally and (having changed https to http), connected via

> nc localhost 9090

GET /demo/sse HTTP/1.1
Host: localhost:9090

I proceed to see a stream of SSE events sent to the client, as expected. Once I close the client via Ctrl-C, I then see the error I'd expect to see in the server:

14:13:55.369 [error] ** (Plug.Conn.WrapperError) ** (MatchError) no match of right hand side value: {:error, "closed"}
    (sse_demo 0.1.0) lib/sse_demo.ex:49: SseDemo.stream_msgs/1
    (sse_demo 0.1.0) deps/plug/lib/plug/router.ex:246: anonymous fn/4 in SseDemo.dispatch/2
    (telemetry 1.2.1) /Users/mat/Desktop/sse_demo/deps/telemetry/src/telemetry.erl:321: :telemetry.span/3
    (sse_demo 0.1.0) deps/plug/lib/plug/router.ex:242: SseDemo.dispatch/2
    (sse_demo 0.1.0) lib/sse_demo.ex:1: SseDemo.plug_builder_call/2
    (bandit 1.5.7) lib/bandit/pipeline.ex:124: Bandit.Pipeline.call_plug!/2
    (bandit 1.5.7) lib/bandit/pipeline.ex:36: Bandit.Pipeline.run/4
    (bandit 1.5.7) lib/bandit/http1/handler.ex:12: Bandit.HTTP1.Handler.handle_data/3

Am I missing something here?

[CharlesOkwuagwu]

Please open in your web browser (MS-edge) https://localhost:9090/demo

[mtrudel]

I see the same behaviour in both Chrome & Safari. Unable to test on Edge, but everything here looks like the server is doing the right thing; I wonder if it's an OS issue (I'm on macOS) or a browser issue? Clients don't always close the underlying connection right away after you close the window, instead keeping it open on a timer for possible reuse. I wonder if that could be what's happening in your case?

[CharlesOkwuagwu]

I see the same behaviour in both Chrome & Safari. Unable to test on Edge, but everything here looks like the server is doing the right thing; I wonder if it's an OS issue (I'm on macOS) or a browser issue? Clients don't always close the underlying connection right away after you close the window, instead keeping it open on a timer for possible reuse. I wonder if that could be what's happening in your case?

WOW you are right, I Was closing only the tab. When I close the browser. the process exits.

Sorry for the disturbance.

[mtrudel]

No worries! Glad it all got figured out!