J0 / phoenix_gen_socket_client

Socket client behaviour for phoenix channels
MIT License
232 stars 48 forks source link

Blocking on connect/join? #26

Closed jbodah closed 6 years ago

jbodah commented 6 years ago

I was just testing this repo out for an internal project, and everything looks great so far but I was surprised that returning {:connect, url, params, state} from the init/1 callback didn't connect synchronously.

Here's a simplified version of my code:

# Callback mod
defmodule WsClient do
  alias Phoenix.Channels.GenSocketClient, as: Ws

  def start_link do
    Ws.start_link(
      __MODULE__,
      Phoenix.Channels.GenSocketClient.Transport.WebSocketClient,
      []
    )
  end

  def init([]) do
    IO.inspect :init
    url = "ws://localhost:4002/socket/websocket"
    topic = "my_topic"
    {:connect, url, [], %{topic: topic}}
  end

  def handle_connected(transport, state) do
    IO.inspect :connected
    {:ok, state}
  end

  def handle_joined(_topic, _payload, _transport, state) do
    IO.inspect :joined
    {:ok, state}
  end

  def handle_call(:join, _from, transport, state) do
    IO.inspect :joining
    Ws.join(transport, state.topic)
    {:reply, :ok, state}
  end

  def handle_call({:push, _message}, _from, _transport, state) do
    IO.inspect :pushing
    {:reply, :ok, state}
  end
end

And when I run the following:

# Client code
{:ok, pid} = WsClient.start_link
WsClient.call(pid, :join)
WsClient.call(pid, {:push, "test"})

I get:

# Output
:init
:joining
:pushing
:connected
:joined

What I'd like to do is the following:

The async connect/join makes this a little trickier however. Is the expected way to handle this to juggle the inbox (e.g. returning {:noreply, state} and then GenServer.reply after we connect)? Am I missing some built-in options? Is there a better way to do this that I'm not seeing?

sasa1977 commented 6 years ago

The reason why connect is not synchronous is because in my opinion it's a bad idea. Keep in mind that just like with any other OTP compliant process (e.g. GenServer) start_link blocks until init/1 has finished. With synchronous connect, the caller (usually a supervisor) would have to wait until the connection is established, which in some cases might take too long. This could affect the total startup time of your entire system.

Even if we supported sync connects, the problem would not be solved, because the question is what should happen if the connection is not established. In this case you can still end up being disconnected when the call arrives. Going further, the same can happen if the connection is broken later on.

So I don't think sync connect would help you here anyway.

If you don't want to juggle with :noreply and GenServer.reply, you could start the rest of the system once you've connected/joined.

To do this, you would start only WsClient from the supervisor. The client process connects, and when it's connected, it joins. Then, in handle_joined, you can ask the parent supervisor to start the rest of the system (e.g. another child which is the supervisor, which starts other workers which use the WsClient). Similarly, on disconnect/leave, you could then ask the parent supervisor to stop these children.

Another option is to simply enqueue pending calls and process them later. If I understand you right, this is what you currently do.

Yet another option is to respond with an error if you're not currently connected/joined.

If you really want to have synchronous connect, you could do this on top of the current functionality. You could do WsClient.start_link(self()), and then have the caller wait for the confirmation message from the client process, which the client would send once it has connected (or joined). Again, I think this is a bad idea, and it won't solve all the problems, but you can give it a try.

Does this answer your question?

jbodah commented 6 years ago

Yes, thanks for the very detailed response. We don't need the response to be synchronous so we're just going to enqueue the message in the process's state and have it flush that when it finally joins the channel