Repository: phoenixframework/phoenix (elixir)

Issue:
Race condition when presence is left shortly after presence state retrieval
<!--
### Precheck

* For help and support, use Stack Overflow or the Phoenix Talk mailing list
* For proposing a new feature, start a discussion on the Phoenix Core mailing list
* For bugs, do a quick search and make sure the bug has not yet been reported
* Ensure that this issue is related to the Phoenix library and not one of the dependencies listed in mix.exs (Ecto, Plug, etc.)
* All checked? Be nice and have fun!
-->

### Environment

* Elixir version (elixir -v): Elixir 1.4.0, erts-8.2)
* Phoenix version (mix deps): phoenix 1.3.0-rc.1
* NodeJS version (node -v): v7.7.2
* NPM version (npm -v): 4.4.1
* Operating system: OS X 10.12.3

### Expected behavior

Using the default setup for channels and presence, any leaves happening concurrently with a new channel's connection should result in a client side presence state without the left presence (either through it not being included in `presence_state` or it being removed shortly thereafter through a `presence_diff`.

### Actual behavior

Some presence changes, that are applied after the newly joined channel has retrieved its current presence state, will be sent to the client before the presence_state event is sent. When this happens, any such presence leaves will be ignored by the default js syncDiff. Once the presence_state event is received, the left presences will be kept even though they are not present anymore.

### Easiest way to reproduce locally

1. Introduce a delay in a `Presence.fetch/2` callback, that scales linearly with the number of presences. Example: `def fetch(_topic, presences) do Process.sleep(1000 * Enum.count presences); presences end`
2. Open a few tabs to make sure the presence state contains 5 or more items.
3. Reload a tab, triggering a new channel connection.
4. Within a few seconds, close one or more of the other tabs.
5. Observe that the newly opened tab recevies the leave events before it's presence_state, and that it keeps those left presences until the next reload.

A runnable example of the above is available at https://github.com/alvinlindstam/phoenix_social_presence/tree/race-condition

I understand that the one second delay is an extreme example, but a fetch function that takes longer for many presences than for a small diff is hardly unlikely. I'm not sure if this would be possible to reproduce without a fetch function, but I can't se any reason why a presence change handled just after a presence state fetch by a new channel couldn't be sent to and handled by the transport process before the channel process itself sends the state to the transport.

Pull request:
Fix race when presence diff arrives before state. Closes #2255

Patch:
diff --git a/assets/js/phoenix.js b/assets/js/phoenix.js
index 07aa3ddd4..df2f03b42 100644
--- a/assets/js/phoenix.js
+++ b/assets/js/phoenix.js
@@ -1140,16 +1140,19 @@ export var Presence = {
 
   syncState(currentState, newState, onJoin, onLeave){
     let state = this.clone(currentState)
+    state.presences = state.presences || {}
+    state.synced = true
+    let pendingDiffs = state.pendingDiffs || []
     let joins = {}
     let leaves = {}
 
-    this.map(state, (key, presence) => {
+    this.map(state.presences, (key, presence) => {
       if(!newState[key]){
         leaves[key] = presence
       }
     })
     this.map(newState, (key, newPresence) => {
-      let currentPresence = state[key]
+      let currentPresence = state.presences[key]
       if(currentPresence){
         let newRefs = newPresence.metas.map(m => m.phx_ref)
         let curRefs = currentPresence.metas.map(m => m.phx_ref)
@@ -1167,26 +1170,44 @@ export var Presence = {
         joins[key] = newPresence
       }
     })
-    return this.syncDiff(state, {joins: joins, leaves: leaves}, onJoin, onLeave)
+
+    state = this.syncDiff(state, {joins: joins, leaves: leaves}, onJoin, onLeave)
+
+    pendingDiffs.forEach(diff => {
+      state = this.syncDiff(state, {joins: diff.joins, leaves: diff.leaves},
+        diff.onJoin,
+        diff.onLeave)
+    })
+    state.pendingDiffs = []
+
+    return state
   },
 
   syncDiff(currentState, {joins, leaves}, onJoin, onLeave){
     let state = this.clone(currentState)
+    state.presences = state.presences || {}
+    state.pendingDiffs = state.pendingDiffs || []
+
+    if(!state.synced){
+      state.pendingDiffs.push({joins, leaves, onJoin, onLeave})
+      return state
+    }
+
     if(!onJoin){ onJoin = function(){} }
     if(!onLeave){ onLeave = function(){} }
 
     this.map(joins, (key, newPresence) => {
-      let currentPresence = state[key]
-      state[key] = newPresence
+      let currentPresence = state.presences[key]
+      state.presences[key] = newPresence
       if(currentPresence){
-        let joinedRefs = state[key].metas.map(m => m.phx_ref)
+        let joinedRefs = state.presences[key].metas.map(m => m.phx_ref)
         let curMetas = currentPresence.metas.filter(m => joinedRefs.indexOf(m.phx_ref) < 0)
-        state[key].metas.unshift(...curMetas);
+        state.presences[key].metas.unshift(...curMetas);
       }
       onJoin(key, currentPresence, newPresence)
     })
     this.map(leaves, (key, leftPresence) => {
-      let currentPresence = state[key]
+      let currentPresence = state.presences[key]
       if(!currentPresence){ return }
       let refsToRemove = leftPresence.metas.map(m => m.phx_ref)
       currentPresence.metas = currentPresence.metas.filter(p => {
@@ -1194,13 +1215,14 @@ export var Presence = {
       })
       onLeave(key, currentPresence, leftPresence)
       if(currentPresence.metas.length === 0){
-        delete state[key]
+        delete state.presences[key]
       }
     })
     return state
   },
 
-  list(presences, chooser){
+  list(state, chooser){
+    let presences = state.presences || {}
     if(!chooser){ chooser = function(key, pres){ return pres } }
 
     return this.map(presences, (key, presence) => {


Test patch:
diff --git a/assets/test/presence_test.js b/assets/test/presence_test.js
index ddef76b42..4bff6e8c0 100644
--- a/assets/test/presence_test.js
+++ b/assets/test/presence_test.js
@@ -29,12 +29,13 @@ describe("syncState", () => {
     assert.deepEqual(state, stateBefore)
 
     state = Presence.syncState(state, newState)
-    assert.deepEqual(state, newState)
+    assert.deepEqual(state.presences, newState)
   })
 
   it("onJoins new presences and onLeave's left presences", () => {
     let newState = fixtures.state()
-    let state = {u4: {metas: [{id: 4, phx_ref: "4"}]}}
+    let state = {}
+    state = Presence.syncState(state, {u4: {metas: [{id: 4, phx_ref: "4"}]}})
     let joined = {}
     let left = {}
     let onJoin = (key, current, newPres) => {
@@ -43,12 +44,9 @@ describe("syncState", () => {
     let onLeave = (key, current, leftPres) => {
       left[key] = {current: current, leftPres: leftPres}
     }
-    let stateBefore = clone(state)
-    Presence.syncState(state, newState, onJoin, onLeave)
-    assert.deepEqual(state, stateBefore)
 
     state = Presence.syncState(state, newState, onJoin, onLeave)
-    assert.deepEqual(state, newState)
+    assert.deepEqual(state.presences, newState)
     assert.deepEqual(joined, {
       u1: {current: null, newPres: {metas: [{id: 1, phx_ref: "1"}]}},
       u2: {current: null, newPres: {metas: [{id: 2, phx_ref: "2"}]}},
@@ -61,7 +59,7 @@ describe("syncState", () => {
 
   it("onJoins only newly added metas", () => {
     let newState = {u3: {metas: [{id: 3, phx_ref: "3"}, {id: 3, phx_ref: "3.new"}]}}
-    let state = {u3: {metas: [{id: 3, phx_ref: "3"}]}}
+    let state = {presences: {u3: {metas: [{id: 3, phx_ref: "3"}]}}}
     let joined = {}
     let left = {}
     let onJoin = (key, current, newPres) => {
@@ -71,7 +69,7 @@ describe("syncState", () => {
       left[key] = {current: current, leftPres: leftPres}
     }
     state = Presence.syncState(state, newState, onJoin, onLeave)
-    assert.deepEqual(state, newState)
+    assert.deepEqual(state.presences, newState)
     assert.deepEqual(joined, {
       u3: {current: {metas: [{id: 3, phx_ref: "3"}]},
            newPres: {metas: [{id: 3, phx_ref: "3"}, {id: 3, phx_ref: "3.new"}]}}
@@ -84,34 +82,62 @@ describe("syncDiff", () => {
   it("syncs empty state", () => {
     let joins = {u1: {metas: [{id: 1, phx_ref: "1"}]}}
     let state = {}
+    state = Presence.syncState(state, {})
     Presence.syncDiff(state, {joins: joins, leaves: {}})
-    assert.deepEqual(state, {})
+    assert.deepEqual(state, {synced: true, presences: {}, pendingDiffs: []})
 
     state = Presence.syncDiff(state, {
       joins: joins,
       leaves: {}
     })
-    assert.deepEqual(state, joins)
+    assert.deepEqual(state.presences, joins)
+  })
+
+  it("applies pending diff if state is not yet synced", () => {
+    let newState = {u1: {metas: [{id: 1, phx_ref: "1"}]},
+                    u2: {metas: [{id: 2, phx_ref: "2"}]}}
+    let leaves = {u2: {metas: [{id: 2, phx_ref: "2"}]}}
+    let state = {}
+    state = Presence.syncDiff(state, {joins: {}, leaves: leaves})
+    assert.deepEqual(state, {
+      presences: {},
+      pendingDiffs: [{
+        joins: {}, leaves: leaves, onJoin: undefined, onLeave: undefined
+      }]
+    })
+
+    state = Presence.syncState(state, newState)
+    assert.deepEqual(state, {
+      synced: true,
+      pendingDiffs: [],
+      presences: {u1: {metas: [{id: 1, phx_ref: "1"}]}}
+    })
   })
 
   it("removes presence when meta is empty and adds additional meta", () => {
-    let state = fixtures.state()
+    let state = {}
+    state = Presence.syncState(state, fixtures.state())
     state = Presence.syncDiff(state, {joins: fixtures.joins(), leaves: fixtures.leaves()})
 
     assert.deepEqual(state, {
-      u1: {metas: [{id: 1, phx_ref: "1"}, {id: 1, phx_ref: "1.2"}]},
-      u3: {metas: [{id: 3, phx_ref: "3"}]}
+      pendingDiffs: [],
+      synced: true,
+      presences: {
+        u1: {metas: [{id: 1, phx_ref: "1"}, {id: 1, phx_ref: "1.2"}]},
+        u3: {metas: [{id: 3, phx_ref: "3"}]}
+      }
     })
   })
 
   it("removes meta while leaving key if other metas exist", () => {
-    let state = {
-      u1: {metas: [{id: 1, phx_ref: "1"}, {id: 1, phx_ref: "1.2"}]}
-    }
+    let state = {}
+    state = Presence.syncState(state, {u1: {metas: [{id: 1, phx_ref: "1"}, {id: 1, phx_ref: "1.2"}]}})
     state = Presence.syncDiff(state, {joins: {}, leaves: {u1: {metas: [{id: 1, phx_ref: "1"}]}}})
 
     assert.deepEqual(state, {
-      u1: {metas: [{id: 1, phx_ref: "1.2"}]},
+      pendingDiffs: [],
+      synced: true,
+      presences: {u1: {metas: [{id: 1, phx_ref: "1.2"}]}},
     })
   })
 })
@@ -119,7 +145,9 @@ describe("syncDiff", () => {
 
 describe("list", () => {
   it("lists full presence by default", () => {
-    let state = fixtures.state()
+    let state = {}
+    state = Presence.syncState(state, fixtures.state())
+
     assert.deepEqual(Presence.list(state), [
       {metas: [{id: 1, phx_ref: "1"}]},
       {metas: [{id: 2, phx_ref: "2"}]},
@@ -128,10 +156,16 @@ describe("list", () => {
   })
 
   it("lists with custom function", () => {
-    let state = {u1: {metas: [
-      {id: 1, phx_ref: "1.first"},
-      {id: 1, phx_ref: "1.second"}]
-    }}
+    let state = {
+      presences: {
+        u1: {
+          metas: [
+            {id: 1, phx_ref: "1.first"},
+            {id: 1, phx_ref: "1.second"}
+          ]
+        }
+      }
+    }
 
     let listBy = (key, {metas: [first, ...rest]}) => {
       return first