Sync channel messaging with new push API #737

chrismccord · 2015-03-31T03:07:19Z

We've overhauled the channel API to allow "synchronous" messaging, and I really love the results. By synchronous, I mean being able to reply to an incoming event directly, while ensuring messaging ordering for the same incoming events. This not only lets you do proper request/response messaging where necessary, but it also fixes issues we have in our <= 0.10 apis where joins were not synchronous and messages could be dropped if you fired them before you were fully joined. With these changes, we have a few high-level concepts which make up channels:

The client and server push messages down the socket to communicate
The server can reply directly to a pushed message
The server can broadcast events to be pushed to all subscribers

The flows looks like this:

client push("ev1") -> server handle_in("ev1") -> server push("ev2") -> client on("ev2")
client push("ev1") -> server handle_in("ev1") -> server broadcast("ev2") -> N subscribers handle_out("ev2") -> N subscribers push("ev2") -> N clientson("ev2")`
client push("ev1") -> server handle_in("ev") -> server {:reply, :ok, ...} -> client receive("ok", ...)

Now let's see some cli/server code:

    socket.join("rooms:lobby", {})
      .after(5000, () => console.log("We're having trouble connecting...") )
      .receive("ignore", () => console.log("auth error") )
      .receive("ok", chan => {

        // can now bind to channel crash/close events since channels are own processes
        chan.onClose( () => console.log("The channel disconnected") )
        chan.onError( () => console.log("The channel crashed!") )

        $input.onEnter( e => {
          // push without response
          chan.push("new_msg", {body: e.text, user: currentUser}) 
        })

        chan.on("status_change", ({status}) => $status.html(status) )

        chan.on("new_msg", msg => $messages.append(msg) )

        // push with `receive`'d response, and optional `after` hooks
        $createNotice.onClick( e => {
          chan.push("create_notice", e.data)
              .receive("ok", notice =>  console.log("notice created", notice) )
              .receive("error", reasons =>  console.log("creation failed", reasons) )
              .after(5000, () => console.log("network interruption") )
        })
    })

defmodule Chat.RoomChannel do
  use Phoenix.Channel

  def join("rooms:lobby", message, socket) do
    send(self, {:after_join, message})

    {:ok, socket}
  end
  def join("rooms:" <> _private_subtopic, _message, _socket) do
    :ignore
  end

  def handle_info({:after_join, msg}, socket) do
    broadcast! socket, "user_entered", %{user: msg["user"]}
    push socket, "status_change", %{status: "waiting for users"}
    {:noreply, socket}
  end

  def handle_in("create_notice", attrs, socket) do
    changeset = Notice.changeset(%Notice{}, attrs)

    if changeset.valid? do
      Repo.insert(changeset)
      {:reply, {:ok, changeset}, socket}
    else
      {:reply, {:error, changeset.errors}, socket}
    end
  end

  def handle_in("new_msg", msg, socket) do
    broadcast! socket, "new_msg", %{user: msg["user"], body: msg["body"]}
    {:noreply, socket}
  end

  # this is forward by the default `handle_out`, but show here for clarity
  def handle_out("new_msg", msg, socket) do
    push socket, "new_msg, msg
    {:noreply, socket}
  end
end

Note that {:reply, {:ok, resp}, socket} on the server, triggers .receive("ok", resp => {}) on the client. The "status" of the reply can be anything, ie {:reply, {:queued, resp}, socket} on the server, triggers .receive("queued", resp => { }) on the client.
Also note that client joining, push, and receiving replies all have the same semantics and API now, which is quite nice.

All feedback appreciated before I merged this upstream. Thanks!

josevalim · 2015-03-31T15:33:24Z

lib/phoenix/channel.ex

@@ -137,14 +137,18 @@ defmodule Phoenix.Channel do
                                                                  :ignore |
                                                                  {:error, reason :: term, Socket.t}

-  defcallback leave(msg :: map, Socket.t) :: {:ok, Socket.t}
+  defcallback leave(msg :: map, Socket.t) :: :ok | {:error, reason :: term}


What was our conclusion regarding leave? I think we were discussing to rename it to terminate, weren't we? I don't remember the rationale anymore.

Ah, I remember. Leave is not always called... only if we trap exits. So I would definitely go with terminate as we have the same definition in there too.

josevalim · 2015-03-31T15:42:19Z

Beautiful. I love the join example! ❤️ 💚 💙 💛 💜

jeregrine · 2015-03-31T15:51:23Z

web/static/js/phoenix.js

+    this.bindings   = []
+    this.afterHooks = []
+    this.recHooks   = {}
+    this.joinPush   = new Push(this, "phx_join", this.message)


Should we move this into a constant? All of these strings.

stoodder · 2015-03-31T16:04:03Z

Dig the usage of ES6 here. You might consider adding semicolons to make this more reliable for minifiers or for people who concatenate all of their vendor javascript code in to a single file (points at self). Not really strongly opinionated on this point though.

chrismccord · 2015-03-31T16:07:27Z

Dig the usage of ES6 here. You might consider adding semicolons to make this more reliable for minifiers or for people who concatenate all of their vendor javascript code in to a single file (points at self).

Thanks! Babel inserts semicolons in the compiled priv/static/phoenix.js, so not an issue for minifiers.

stoodder · 2015-03-31T16:07:53Z

Perfect! Then no worries!

paulcsmith · 2015-03-31T17:50:08Z

This is outstanding work. The examples are very clear 👍

If returning {:noreply, socket} in handle_in is the most common scenario, is it a good idea for broadcast! (and maybe push?) to return {:no_reply, socket} so that developers don't need to type that every time? Or is {:ok, socket} still most common?
It seems that broadcast! is preferred over broadcast. The docs do not explain why that is. You could infer that it's because it throws an error if there is a failure, but it's not clear. It also seems odd (coming from Ruby) that the most used version is the ! version. I'm sure there are good reasons for this, but it does seem counter-intuitive to me.

chrismccord · 2015-03-31T17:53:36Z

If returning {:noreply, socket} in handle_in is the most common scenario, is it a good idea for broadcast! (and maybe push?) to return {:no_reply, socket} so that developers don't need to type that every time? Or is {:ok, socket} still most common?

I considered this because before I had broadcast/3 and reply/3 both return {:ok, socket}, but now with the true {:reply vs {:noreply, semantics, I think explicit is better. What are other's thoughts?

It seems that broadcast! is preferred over broadcast. The docs do not explain why that is.

I will bump the docs to better explain why. Your guess is correct. If the pubsub server is down, you likely want to crash, but there are cases where you may only want to optimistically broadcast but messages being dropped aren't mission critical.

paulcsmith · 2015-03-31T18:00:50Z

I considered this because before I had broadcast/3 and reply/3 both return {:ok, socket}, but now with the true {:reply vs {:noreply, semantics, I think explicit is better. What are other's thoughts?

Knowing this, I think it makes sense to return :noreply as a default and overriding only when you mean to reply. I would not expect push and broadcast to reply, so returning :noreply makes sense to me.

BlakeWilliams · 2015-03-31T18:24:42Z

I can see it going both ways, but I agree with @paulcsmith here since there is the explicit reply and noreply I wouldn't expect push and broadcast to reply for you.

I think there might be value in making two methods, push_and_reply and broadcast_and_reply to make the distinction more clear.

josevalim · 2015-03-31T18:45:01Z

👍 to being explicit. The implicit return would make your code suddenly
fail by moving the lines around. For example you decide to log at the last
line and won't realize until later because of the implicit return.

How would push_and_reply help though? Wouldn't you need to pass the
contents you need to reply to anyway? If so, the tuple is definitely better.

Should we also change broadcast and push to just return :ok?

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Lead Developer

BlakeWilliams · 2015-03-31T18:49:24Z

That's a good point, unless you pass both it wouldn't make much sense. In that case I'd lean towards being explicit each time.

chrismccord · 2015-03-31T18:49:56Z

The implicit return would make your code suddenly fail by moving the lines around.

I think this argument seals it. Explicit > implicit here.

Should we also change broadcast and push to just return :ok?

👍

josevalim · 2015-03-31T19:05:14Z

We do have the same failure with the tuple but it is much more unlikely to
happen because it is explicit and it will remind you.

Generally speaking, moving code around will cause breakage, the goal is
exactly to make those explicit so it will happen less often!

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Lead Developer

paulcsmith · 2015-03-31T19:06:44Z

Oops, sorry I deleted my comment. I saw that Chris already made a decision so I deleted it.

You have a great point. Less breakage is good :)

josevalim · 2015-03-31T19:17:41Z

That's ok! I am stuck on mobile, so I get to see everything :P

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Lead Developer

paulcsmith · 2015-03-31T19:18:18Z

test/phoenix/channel_test.exs

@@ -157,7 +158,7 @@ defmodule Phoenix.ChannelTest do
  end

  test "#broadcast_from and #broadcast_from! raises error when msg isn't a Map" do
-    socket = Socket.put_topic(new_socket, "top:subtop")
+    socket = new_socket |> put_in([:joined], true) |> Socket.put_topic("top:subtop")


new_socket |> put_in([:joined], true) is used fairly often. Maybe extract a function called new_joined_socket, or joined_socket?

Yeah. This whole test file needs cleaned up :)

BlakeWilliams · 2015-03-31T20:27:06Z

web/static/js/phoenix.js

+  // Return the next message ref, accounting for overflows
+  makeRef(){
+    let newRef = this.ref + 1
+    if(newRef === this.ref){ this.ref = 0 } else { this.ref = newRef }


Expand this for clarity?

scrogson · 2015-03-31T22:41:24Z

Beautiful work @chrismccord! Can't wait to use it.

Teardown/cleanup should be handled by trapping exits and handling in terminate/2

chrismccord added 5 commits March 28, 2015 00:27

WIP channel replies with client refs

7bf8920

DRY up close/error events for transports

6352e9b

Add ability to reply on server and pick up on client

117e82c

Bump docs and update channel return signatures

39619c1

Rate limit auto channel rejoining after crashes

8c260fb

chrismccord mentioned this pull request Mar 31, 2015

Discuss sync abstractions on top of channels #674

Closed

josevalim reviewed Mar 31, 2015
View reviewed changes

jeregrine reviewed Mar 31, 2015
View reviewed changes

paulcsmith reviewed Mar 31, 2015
View reviewed changes

BlakeWilliams reviewed Mar 31, 2015
View reviewed changes

chrismccord added 6 commits March 31, 2015 21:28

Unify channel event names

9aa58f7

phoenix_live_reload reminder

e56b07c

Add :stop return signatures

6b3efb1

Special-case phx_leave event to stop server

fa16902

Teardown/cleanup should be handled by trapping exits and handling in terminate/2

Require a ref for all messages

2333c11

Make client leave a sync event

b53eb1e

chrismccord merged commit b53eb1e into master Apr 3, 2015

chrismccord deleted the cm-sync-channels branch April 9, 2015 02:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync channel messaging with new push API #737

Sync channel messaging with new push API #737

chrismccord commented Mar 31, 2015

josevalim Mar 31, 2015

josevalim Mar 31, 2015

josevalim commented Mar 31, 2015

jeregrine Mar 31, 2015

chrismccord Mar 31, 2015

stoodder Mar 31, 2015

stoodder commented Mar 31, 2015

chrismccord commented Mar 31, 2015

stoodder commented Mar 31, 2015

paulcsmith commented Mar 31, 2015

chrismccord commented Mar 31, 2015

paulcsmith commented Mar 31, 2015

BlakeWilliams commented Mar 31, 2015

josevalim commented Mar 31, 2015

BlakeWilliams commented Mar 31, 2015

chrismccord commented Mar 31, 2015

josevalim commented Mar 31, 2015

paulcsmith commented Mar 31, 2015

josevalim commented Mar 31, 2015

paulcsmith Mar 31, 2015

chrismccord Mar 31, 2015

BlakeWilliams Mar 31, 2015

scrogson commented Mar 31, 2015

Sync channel messaging with new push API #737

Sync channel messaging with new push API #737

Conversation

chrismccord commented Mar 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented Mar 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoodder commented Mar 31, 2015

chrismccord commented Mar 31, 2015

stoodder commented Mar 31, 2015

paulcsmith commented Mar 31, 2015

chrismccord commented Mar 31, 2015

paulcsmith commented Mar 31, 2015

BlakeWilliams commented Mar 31, 2015

josevalim commented Mar 31, 2015

BlakeWilliams commented Mar 31, 2015

chrismccord commented Mar 31, 2015

josevalim commented Mar 31, 2015

paulcsmith commented Mar 31, 2015

josevalim commented Mar 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scrogson commented Mar 31, 2015