Plug.Conn is not closed when user closes the browser's tab (SSE)

[mickel8]

Hi,
I have implemented SSE (Server-Sent-Events) using Plug and Bandit, and they work really cool until a user closes browser's tab. For some reason, my Plug.Conn isn't notified/invalidated and I can still send data (although it doesn't appear in Wireshark). The code looks more or less like this

 get "/api/resource/:resource_id/sse/event-stream" do
    viewers = PeerConnection.get_all_running() |> length()

    {:ok, conn} =
      conn
      |> put_resp_header("content-type", "text/event-stream")
      |> put_resp_header("cache-control", "no-cache")
      |> send_chunked(200)
      |> chunk("data: {\"viewerscount\": #{viewers}}\n\n")

    update_viewers_count(conn, self())
  end


  defp update_viewers_count(conn, pid) do
    viewers = PeerConnection.get_all_running() |> length()
    Process.send_after(self(), :update_viewerscount, @viewers_count_update_interval)

    receive do
      :update_viewerscount ->
        {:ok, conn} = chunk(conn, "data: {\"viewerscount\": #{viewers}}\n\n")
        update_viewers_count(conn, pid)
    end
  end

When I switched to the cowboy, everything works as expected, i.e. once the tab is closed, a process handling a connection on the server side is also closed.

[mtrudel]

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 High Memory usage for DelegatingHandler.init/1 #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 Stopping the request handler #202.

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

Thanks all!

[mickel8]

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

[mtrudel]

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

[mickel8]

@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

[mickel8]

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

[mickel8]

@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.

[mickel8]

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

[mickel8]

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

[mtrudel]

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.

[mickel8]

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.

[mtrudel]

@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"}

[mickel8]

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.

[mickel8]

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

[mtrudel]

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!