mtrudel / bandit

Bandit is a pure Elixir HTTP server for Plug & WebSock applications
MIT License
1.7k stars 85 forks source link

SSE connections are not killing process after handler is finished #399

Closed eliasdarruda closed 1 week ago

eliasdarruda commented 2 months ago

For the given SSE endpoint:

defmodule SomeTestWeb.Controller do
  use SomeTestWeb, :controller

  def sse(conn, params) do
    conn =
      conn
      |> put_resp_header("content-type", "text/event-stream")
      |> put_resp_header("x-accel-buffering", "no")
      |> send_chunked(200)

    # adds the current process to the "sse_connection" :pg group
    :pg.join("sse_connection", self())

    parent_pid = self()

    # Spawns a process that will monitor whenever this current request is killed
    spawn(fn ->
      Process.monitor(parent_pid)

      receive do
        {:DOWN, _ref, :process, _object, _reason} ->
          # leave :pg group due to connection being down, either by client or timeout
          :pg.leave("sse_connection", parent_pid)
      end
    end)

    loop_connection_until_event(conn)
  end

  defp loop_connection_until_event(conn) do
    receive do
      {:close_with_data, data} ->
        send_message(conn, data)

        conn

      _event ->
        loop_connection_until_event(conn)
    end
  end

  defp send_message(conn, data) do
    chunk(conn, "event: \"message\"\n\ndata: #{data}\n\n")
  end
end

You can call this endpoint using whatever client you want.

Since the process is now attached with a :pg group, we can now check the connected member and send a message directly through the process.

iex(1)> :pg.get_members("sse_connection")
[#PID<0.51564.0>]
iex(2)> :pg.get_members("sse_connection") |> Enum.at(0) |> Process.send({:close_with_data, "some_data"}, [])
:ok
iex(3)> Process.alive?(pid(0, 51564, 0)) # the process was not killed
true
iex(4)> :pg.get_members("sse_connection") # the process is still in pg queue (Process.monitor was not triggered by it)
[#PID<0.51564.0>]

As described above, it doesn't close the process associated with it.

The expected scenario is that the connection and process is closed after reaching the end of the handler function.

If you try to close using a fetch client in any browser using AbortSignal it doesn't close either.

This is currently blocking me from moving to bandit instead of cowboy adapter. If you try doing that using cowboy adapter it will have this behavior I'm describing.

mtrudel commented 2 months ago

This is most likely because your client is keeping the connection open for keep-alive RFC9112§9.3. If you send the Connection: close response header your client should close the connection upon termination of the stream.

This is a perennially awkward corner of the Plug API; there's been discussion before about how best to disposition the connection at the end of chunk responses but every solution has downsides. Cowboy doesn't exhibit this issue because each single request on a persistent connection is handled by a separate process, so you're not seeing the connection process staying alive in the background. Bandit is rather 'closer to the metal' in that regard.

eliasdarruda commented 2 months ago

@mtrudel Setting Connection: close in the response header properly kills the connection after sending a message. However when clients kill the connection, for example when using fetch with AbortSignal, we have no notification whatsoever in the process that this request was canceled.

I can see the reason that the process itself isn't killed when client closes and what you said makes sense, however, is there a way to have some sort of message sent in this process when it happens? Just like {:plug_conn, :sent} message, maybe something like {:plug_conn, :closed}?

mtrudel commented 1 week ago

AbortSignal doesn't necessarily close the actual underlying TCP connection to the server, only indicates to the browser's network stack that it no longer needs the connection. Nothing guarantees that the browser will actually close the underlying connection since it may elect to keep it open for cache reasons (similar issues have been raised as #359 & #354).

In light of this, Bandit is doing the right thing here. Keeping the connection open for keep-alive is the default on HTTP/1.1 and is what you're seeing.