refactor: propagate update responses before update notifications

collective-soundworks · Dec 12, 2023 · a5b617c · a5b617c
1 parent 251ea94
commit a5b617c
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 28 deletions.
diff --git a/src/common/SharedStatePrivate.js b/src/common/SharedStatePrivate.js
@@ -89,49 +89,46 @@ class SharedStatePrivate {
         }
 
         if (hasUpdates) {
-          // send response to requester
-          // client.transport.emit(`${UPDATE_RESPONSE}-${this.id}-${remoteId}`, reqId, filteredUpdates);
-
-          // @note: we propagate server-side last, because as the server transport
-          // is synchronous it can break ordering if a subscription function makes
-          // itself an update in reaction to an update, therefore network messages
-          // order would be broken,
-
-          // we need to handle cases where:
-          // client state (client.id: 2) sends a request
-          // server attached state (client.id: -1) spot a problem and overrides the value
-          // we want the remote client (id: 2) to receive in the right order:
+          // We need to handle cases where:
+          // - client state (client.id: 2) sends a request
+          // - server attached state (client.id: -1) spot a problem and overrides the value
+          // We want the remote client (id: 2) to receive in the right order:
           // * 1. the value it requested,
           // * 2. the value overriden by the server-side attached state (id: -1)
+          //
+          // such problem is now better solved with the the upateHook system, none
+          // nonetheway we don't want to introduce inconsistencies here
+          //
+          // Then we propagate server-side last, because as the server transport
+          // is synchronous it can break ordering if a subscription function makes
+          // itself an update in reaction to an update. Propagating to server last
+          // alllows to maintain network messages order consistent.
 
-          // this problem could be solved properly with a reducer system:
-          // if (dirty) {
-          //   -> call (async) reducer
-          //   -> get values from reducer
-          //.  -> dispatch to everybody
-          // }
+          // @note - remoteId correspond to unique remote state id
+
+          // propagate RESPONSE to the client that originates the request if not the server
+          if (client.id !== -1) {
+            client.transport.emit(`${UPDATE_RESPONSE}-${this.id}-${remoteId}`, reqId, filteredUpdates, context);
+          }
 
-          for (let [peerRemoteId, peer] of this._attachedClients.entries()) {
-            // propagate notification to all other attached clients except server
+          // propagate NOTIFICATION to all other attached clients except server
+          for (let [peerRemoteId, peer] of this._attachedClients) {
             if (remoteId !== peerRemoteId && peer.id !== -1) {
               peer.transport.emit(`${UPDATE_NOTIFICATION}-${this.id}-${peerRemoteId}`, filteredUpdates, context);
             }
           }
 
-          if (client.id !== -1) {
+          // propagate RESPONSE to server if it is the requester
+          if (client.id === -1) {
             client.transport.emit(`${UPDATE_RESPONSE}-${this.id}-${remoteId}`, reqId, filteredUpdates, context);
           }
 
-          for (let [peerRemoteId, peer] of this._attachedClients.entries()) {
-            // propagate notification to server
+          // propagate NOTIFICATION other attached state that belongs to server
+          for (let [peerRemoteId, peer] of this._attachedClients) {
             if (remoteId !== peerRemoteId && peer.id === -1) {
               peer.transport.emit(`${UPDATE_NOTIFICATION}-${this.id}-${peerRemoteId}`, filteredUpdates, context);
             }
           }
-
-          if (client.id === -1) {
-            client.transport.emit(`${UPDATE_RESPONSE}-${this.id}-${remoteId}`, reqId, filteredUpdates, context);
-          }
         } else {
           // propagate back to the requester that the update has been aborted
           // ignore all other attached clients.

diff --git a/tests/states/StateManager.spec.js b/tests/states/StateManager.spec.js
@@ -151,7 +151,9 @@ describe(`# StateManager`, () => {
       await a0.delete();
     });
 
-    it('should propagate updates to all attached states (client)', async () => {
+    it('should propagate updates to all attached states (client)', async function() {
+      this.timeout(5000);
+
       const a0 = await client.stateManager.create('a');
       const a1 = await client.stateManager.attach('a', a0.id);
       const a2 = await client.stateManager.attach('a', a0.id);
@@ -168,6 +170,7 @@ describe(`# StateManager`, () => {
       for (let state of [a0, a1, a2]) {
         for (let i = 1; i <= 100; i++) {
           await state.set({ int: i });
+          await delay(10);
 
           assert.equal(a0.get('int'), i);
           assert.equal(a1.get('int'), i);