mongo race condition checks

Author: darthfiddler

src/client/multiplayer/multiplayer.js

@@ -85,7 +85,10 @@ export class Multiplayer {
     }
 
     this.socket.on('sync', (gameID, state) => {
-      if (gameID == this.gameID) {
+      if (
+        gameID == this.gameID &&
+        state._stateID >= this.store.getState()._stateID
+      ) {
         const action = ActionCreators.restore(state);
         action._remote = true;
         this.store.dispatch(action);

src/client/multiplayer/multiplayer.test.js

@@ -82,11 +82,15 @@ test('multiplayer', () => {
 
   // sync restores state.
   const restored = { restore: true };
-  expect(store.getState()).not.toEqual(restored);
+  expect(store.getState()).not.toMatchObject(restored);
   mockSocket.receive('sync', 'unknown gameID', restored);
-  expect(store.getState()).not.toEqual(restored);
+  expect(store.getState()).not.toMatchObject(restored);
   mockSocket.receive('sync', 'default:default', restored);
-  expect(store.getState()).toEqual(restored);
+  expect(store.getState()).not.toMatchObject(restored);
+  // Only if the stateID is not stale.
+  restored._stateID = 1;
+  mockSocket.receive('sync', 'default:default', restored);
+  expect(store.getState()).toMatchObject(restored);
 
   // updateGameID causes a sync.
   mockSocket.emit = jest.fn();

src/server/db.js

@@ -89,6 +89,28 @@ export class Mongo {
    * @param {object} store - A game state to persist.
    */
   async set(id, state) {
+    // Don't set a value if the cache has a more recent version.
+    // This can occur due a race condition.
+    //
+    // For example:
+    //
+    // A --sync--> server | DB => 0 --+
+    //                                |
+    // A <--sync-- server | DB => 0 --+
+    //
+    // B --sync--> server | DB => 0 ----+
+    //                                  |
+    // A --move--> server | DB <= 1 --+ |
+    //                                | |
+    // A <--sync-- server | DB => 1 --+ |
+    //                                  |
+    // B <--sync-- server | DB => 0 ----+
+    //
+    const cacheValue = this.cache.get(id);
+    if (cacheValue && cacheValue._stateID >= state._stateID) {
+      return;
+    }
+
     this.cache.set(id, state);
 
     const col = this.db.collection(id);
@@ -105,9 +127,9 @@ export class Mongo {
    *                     if no game is found with this id.
    */
   async get(id) {
-    const item = this.cache.get(id);
-    if (item !== undefined) {
-      return item;
+    let cacheValue = this.cache.get(id);
+    if (cacheValue !== undefined) {
+      return cacheValue;
     }
 
     const col = this.db.collection(id);
@@ -117,7 +139,27 @@ export class Mongo {
       .limit(1)
       .toArray();
 
-    this.cache.set(id, docs[0]);
+    let oldStateID = 0;
+    cacheValue = this.cache.get(id);
+    /* istanbul ignore next line */
+    if (cacheValue !== undefined) {
+      /* istanbul ignore next line */
+      oldStateID = cacheValue._stateID;
+    }
+
+    let newStateID = -1;
+    if (docs.length > 0) {
+      newStateID = docs[0]._stateID;
+    }
+
+    // Update the cache, but only if the read
+    // value is newer than the value already in it.
+    // A race condition might overwrite the
+    // cache with an older value, so we need this.
+    if (newStateID >= oldStateID) {
+      this.cache.set(id, docs[0]);
+    }
+
     return docs[0];
   }
 
@@ -127,8 +169,8 @@ export class Mongo {
    * @returns {boolean} - True if a game with this id exists.
    */
   async has(id) {
-    const item = this.cache.get(id);
-    if (item !== undefined) {
+    const cacheValue = this.cache.get(id);
+    if (cacheValue !== undefined) {
       return true;
     }
 

src/server/db.test.js

@@ -81,3 +81,26 @@ test('Mongo', async () => {
     expect(db.dbname).toBe('test');
   }
 });
+
+test('Mongo - race conditions', async () => {
+  const mockClient = MongoDB.MongoClient;
+  const db = new Mongo({ mockClient, url: 'a' });
+  await db.connect();
+
+  // Out of order set()'s.
+  await db.set('gameID', { _stateID: 1 });
+  await db.set('gameID', { _stateID: 0 });
+  expect(await db.get('gameID')).toEqual({ _stateID: 1 });
+
+  // Do not override cache on get() if it is fresher than Mongo.
+  await db.set('gameID', { _stateID: 0 });
+  db.cache.set('gameID', { _stateID: 1 });
+  await db.get('gameID');
+  expect(db.cache.get('gameID')).toEqual({ _stateID: 1 });
+
+  // Override if it is staler than Mongo.
+  await db.set('gameID', { _stateID: 1 });
+  db.cache.reset();
+  expect(await db.get('gameID')).toMatchObject({ _stateID: 1 });
+  expect(db.cache.get('gameID')).toMatchObject({ _stateID: 1 });
+});

src/server/index.js

@@ -89,8 +89,10 @@ export function Server({ games, db, _clientInfo, _roomInfo }) {
             }
           }
 
-          db.set(gameID, store.getState());
+          await db.set(gameID, store.getState());
         }
+
+        return;
       });
 
       socket.on('sync', async (gameID, playerID, numPlayers) => {
@@ -106,6 +108,7 @@ export function Server({ games, db, _clientInfo, _roomInfo }) {
         clientInfo.set(socket.id, { gameID, playerID });
 
         let state = await db.get(gameID);
+
         if (state === undefined) {
           const store = Redux.createStore(reducer);
           state = store.getState();