Plug.Conn is not closed when user closes the browser's tab (SSE) about bandit HOT 14 CLOSED

I've tightened up the semantics on this and pushed up a proper PR at #346. I'd love feedback to see if this fixes your issues @mickel8, @MrYawe, @hubertlepicki

from bandit.

Sorry for the late response!

I think this may rhyme with #202 and #305. I've pushed up a branch that disables exit trapping on HTTP/1 connections (so they'll close as soon as the underlying socket gets closed remotely). It's up on a test branch at:

{:bandit, "~> 1.0", github: "mtrudel/bandit", branch: "no_trap_exit_experiment"}

Note that the above is just an experiment branch; a couple of telemetry flows may not work 100% as expected. I just want to get a sense of whether this is the correct approach. Specifically for the folks involved in this / adjacent PRs:

• @mickel8 could you try the above and see if its semantics are more aligned with what you expect?
• @MrYawe this is probably something good for you to try as well (in addition to the branch suggested in #339; one / both of those fixes may be what you're looking for).
• @hubertlepicki this is also likely going to give you the semantics you were looking for way back on #202.

In all cases, I'd greatly appreciate objective feedback so I can assess which of the approaches to refine & land.

Thanks all!

from bandit.

Hi @mtrudel, thanks! I will test those changes on Thursday

from bandit.

@mtrudel sorry for the late response. I am testing branch no_trap_exit_experiment but it doesn't help 🤔 Even when I closed the browser, my process still try to send something

from bandit.

I tried both with trapping exits and without. In any case, I am not notified that the process that handles incoming request was closed.

from bandit.

@mtrudel I've created minimal reproducible project: https://github.com/mickel8/bandit_sse

When you run it with mix run --no-halt and go under localhost:5002, you will see that the app is streaming messages. When you close the tab, the app is still trying to send messages.

When you uncomment Plug.Cowboy in bandit_see.ex line 7, the app no longer tries to send data after closing the browser.

from bandit.

Hi @mtrudel!

I have debugged this more and:

• there is no erlang message informing about socket being closed by the peer
• this info is only returned from :gen_tcp.recv and :gen_tcp.send
• all calls to ThousandIsland.Socket.send ignore return values

It looks like that to solve the problem we should check against the return value of ThousandIsland.Socket.send and either propagate {:error, :closed} to the Plug's user or raise as in the case of ThousandIsland.Socket.recv

from bandit.

Let me know what you think. Maybe I could try to submit a PR

from bandit.

What to do with return values from those calls is something I went back and forth on a few times in 1.4. TBH, it's tough to surface this sort of thing via the Plug API; it's a simple abstraction that's easy to understand, but it's a bit at odds with so much of the realtime nature of OTP.

Leave it with me; I'll get a branch up shortly for test.

from bandit.

Perfect! Thanks 🙂

Just out of curiosity. Isn't returning {:error, :closed} tuple desired here? Is it against OTP design prinicples? Plug even have an example for chunk function that suggests {:error, :closed} can be returned from the underlying adapter.

from bandit.

@mickel8 can you try the branch in #359 and see if it helps you?

{:bandit, "~> 1.0", github: "mtrudel/bandit", branch: "error_on_send_error"}

from bandit.

For sure messages are no longer sent but now I am getting the following error

12:58:40.292 [error] ** (Plug.Conn.WrapperError) ** (MatchError) no match of right hand side value: {:error, "** (Bandit.HTTPError) closed"}
    (bandit_sse 0.1.0) lib/router.ex:40: BanditSse.Router.stream_msgs/1
    (bandit_sse 0.1.0) deps/plug/lib/plug/router.ex:246: anonymous fn/4 in BanditSse.Router.dispatch/2
    (telemetry 1.2.1) /home/michal/Repos/bandit_sse/deps/telemetry/src/telemetry.erl:321: :telemetry.span/3
    (bandit_sse 0.1.0) deps/plug/lib/plug/router.ex:242: BanditSse.Router.dispatch/2
    (bandit_sse 0.1.0) lib/router.ex:1: BanditSse.Router.plug_builder_call/2
    (bandit 1.5.2) lib/bandit/pipeline.ex:124: Bandit.Pipeline.call_plug!/2
    (bandit 1.5.2) lib/bandit/pipeline.ex:36: Bandit.Pipeline.run/4
    (bandit 1.5.2) lib/bandit/http1/handler.ex:12: Bandit.HTTP1.Handler.handle_data/3

If you want to, you can run this example: https://github.com/mickel8/bandit_sse
just do mix run --no-halt and go under localhost:5002.

from bandit.

Sorry, I misread the log. It's from my code. Yeap, looks that everything works except that the error tuple returned from chunk has a string as its second element

from bandit.

Yeah, the main thrust of this work is to better surface errors on sending, so it's expected that you'll now be seeing those errors (previously we just silently ate them).

I'll update the text of that error; should only be sending the underlying exception's message as the reason.

Thanks for testing!

from bandit.