fix 'bad checksum' issue.

When a second client used to connect to a room, it was receiving the last state of the room, instead of the last snapshot state other users already have. After receiving the first patch broadcast, the new client used to have a 'bad checksum' error in the client-side, because the patch used the previous sent state snapshot. TLDR: The clients must share a common snapshot to be able to apply the patch.
colyseus · Feb 6, 2017 · d8a94ce · d8a94ce
1 parent 480d2dd
commit d8a94ce
Show file tree

Hide file tree

Showing 5 changed files with 41 additions and 145 deletions.
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "colyseus",
-  "version": "0.4.10",
+  "version": "0.4.11",
   "description": "Multiplayer Game Server for Node.js.",
   "main": "./lib/index.js",
   "typings": "./lib/index.d.ts",
@@ -49,6 +49,8 @@
   },
   "license": "MIT",
   "greenkeeper": {
-    "ignore": ["mocha"]
+    "ignore": [
+      "mocha"
+    ]
   }
 }
diff --git a/src/Room.ts b/src/Room.ts
@@ -22,7 +22,12 @@ export abstract class Room<T> extends EventEmitter {
   public options: any;
 
   public state: T;
+
+  // when a new user connects, it receives the '_previousState', which holds
+  // the last binary snapshot other users already have, therefore the patches
+  // that follow will be the same for all clients.
   protected _previousState: any;
+  protected _previousStateEncoded: any;
 
   private _simulationInterval: NodeJS.Timer;
   private _patchInterval: number;
@@ -72,7 +77,10 @@ export abstract class Room<T> extends EventEmitter {
   public setState (newState) {
     this.clock.start();
 
-    this._previousState = this.getEncodedState();
+    // ensure state is populated for `sendState()` method.
+    this._previousState = toJSON( newState );
+    this._previousStateEncoded = msgpack.encode( this._previousState );
+
     this.state = newState;
 
     if ( this.timeline ) {
@@ -122,42 +130,36 @@ export abstract class Room<T> extends EventEmitter {
     client.send( msgpack.encode( [
       Protocol.ROOM_STATE,
       this.roomId,
-      toJSON( this.state ),
+      this._previousState,
       this.clock.currentTime,
       this.clock.elapsedTime,
     ] ), {
       binary: true
     }, logError.bind(this) );
   }
 
-  private broadcastState (): boolean {
-    return this.broadcast( msgpack.encode([
-      Protocol.ROOM_STATE,
-      this.roomId,
-      toJSON( this.state )
-    ]) );
-  }
-
   private broadcastPatch (): boolean {
     if ( !this._previousState ) {
       throw new Error( 'trying to broadcast null state. you should call #setState on constructor or during user connection.' );
     }
 
-    let newState = this.getEncodedState();
+    let currentState = toJSON( this.state );
+    let currentStateEncoded = msgpack.encode( currentState );
 
     // skip if state has not changed.
-    if ( newState.equals( this._previousState ) ) {
+    if ( currentStateEncoded.equals( this._previousStateEncoded ) ) {
       return false;
     }
 
-    let patches = fossilDelta.create( this._previousState, newState );
+    let patches = fossilDelta.create( this._previousStateEncoded, currentStateEncoded );
 
     // take a snapshot of the current state
     if (this.timeline) {
       this.timeline.takeSnapshot( this.state, this.clock.elapsedTime );
     }
 
-    this._previousState = newState;
+    this._previousState = currentState;
+    this._previousStateEncoded = currentStateEncoded;
 
     // broadcast patches (diff state) to all clients,
     // even if nothing has changed in order to calculate PING on client-side
@@ -202,8 +204,4 @@ export abstract class Room<T> extends EventEmitter {
     }
   }
 
-  private getEncodedState () {
-    return msgpack.encode( toJSON( this.state ) );
-  }
-
 }
diff --git a/test/PatchTest.ts b/test/PatchTest.ts
@@ -4,7 +4,7 @@ import { Room } from "../src/Room";
 import { createDummyClient, DummyRoom } from "./utils/mock";
 import { Protocol } from "../src/Protocol";
 
-describe('Room patches', function() {
+describe('Patch', function() {
   let room: Room<any>;
 
   beforeEach(function() {
@@ -28,114 +28,18 @@ describe('Room patches', function() {
     })
   })
 
-  describe('#sendState/#broadcastState', function() {
-    it('should send state when it is set up', function() {
-      let room = new DummyRoom({ });
-      let client = createDummyClient();
-      (<any>room)._onJoin(client, {});
-
-      room.setState({ success: true });
-
-      // first message
-      (<any>room).sendState(client);
-
-      var message = msgpack.decode( client.messages[1] );
-      assert.equal(message[0], Protocol.ROOM_STATE);
-      assert.deepEqual(message[2], { success: true });
-
-      // second message
-      (<any>room).broadcastState();
-
-      var message = msgpack.decode( client.messages[2] );
-      assert.equal(message[0], Protocol.ROOM_STATE);
-      assert.deepEqual(message[2], { success: true });
-    })
-
+  describe('#sendState', function() {
     xit('should allow null and undefined values', function() {
       let room = new DummyRoom({ });
       let client = createDummyClient();
       (<any>room)._onJoin(client, {});
 
       room.setState({ n: null, u: undefined });
 
-      (<any>room).broadcastState();
-
       var message = msgpack.decode( client.messages[1] );
       assert.equal(message[0], Protocol.ROOM_STATE);
       assert.deepEqual(message[2], { n: null, u: undefined });
     })
   })
 
-  describe('#broadcastPatch', function() {
-    it('shouldn\'t broadcast patch with no state or no patches', function() {
-      let room = new DummyRoom({ });
-
-      // connect 2 dummy clients into room
-      let client1 = createDummyClient();
-      (<any>room)._onJoin(client1, {});
-
-      let client2 = createDummyClient();
-      (<any>room)._onJoin(client2, {});
-
-      assert.equal(undefined, room.state);
-
-      // broadcasting without having state should throw error
-      assert.throws(function () {
-        (<any>room).broadcastPatch();
-      });
-
-      // ideally patches should be empty if nothing has changed
-      assert.equal( 1, client1.messages.length )
-
-      room.setState({one: 1})
-      assert.deepEqual({one: 1}, room.state)
-      assert.equal(true, (<any>room).broadcastPatch())
-    })
-
-    it('shouldn\'t broadcast clean state (no patches)', function() {
-      var room = new DummyRoom({ });
-      room.setState({ one: 1 });
-
-      // create 2 dummy connections with the room
-      var client = createDummyClient();
-      (<any>room)._onJoin(client, {});
-
-      var client2 = createDummyClient();
-      (<any>room)._onJoin(client2, {});
-
-      assert.deepEqual({one: 1}, room.state);
-      assert.equal(true, (<any>room).broadcastPatch());
-
-      room.state.two = 2;
-      assert.deepEqual({one: 1, two: 2}, room.state);
-      assert.equal(true, (<any>room).broadcastPatch());
-
-      assert.equal(client.messages.length, 4);
-      assert.equal(client2.messages.length, 4);
-
-      // first message, join room
-      var message = msgpack.decode(client.messages[0]);
-      assert.equal(message[0], Protocol.JOIN_ROOM);
-
-      // second message, room state
-      var message = msgpack.decode(client.messages[1]);
-      assert.equal(message[0], Protocol.ROOM_STATE);
-
-      // third message, empty patch state
-      var message = msgpack.decode(client.messages[2]);
-      assert.equal(message[0], Protocol.ROOM_STATE_PATCH);
-      assert.deepEqual(message[2].length, 17);
-      // // TODO: ideally empty patch state should have 0 length
-      // assert.deepEqual(message[2].length, 0)
-
-      // fourth message, room patch state
-      var message = msgpack.decode(client.messages[3]);
-      assert.equal(message[0], Protocol.ROOM_STATE_PATCH);
-
-      assert.deepEqual(message[2], [ 66, 10, 66, 58, 130, 163, 111, 110, 101, 1, 163, 116, 119, 111, 2, 49, 86, 53, 49, 74, 89, 59 ]);
-    })
-  })
-
 });
-
-
diff --git a/test/RoomTest.ts b/test/RoomTest.ts
@@ -83,7 +83,7 @@ describe('Room', function() {
     });
   });
 
-  describe('#sendState/#broadcastState', function() {
+  describe('#sendState', function() {
     it('should send state when it is set up', function() {
       let room = new DummyRoom({ });
       let client = createDummyClient();
@@ -94,14 +94,9 @@ describe('Room', function() {
       // first message
       (<any>room).sendState(client);
 
-      var message = msgpack.decode( client.messages[1] );
-      assert.equal(message[0], Protocol.ROOM_STATE);
-      assert.deepEqual(message[2], { success: true });
 
-      // second message
-      (<any>room).broadcastState();
+      var message = msgpack.decode( client.messages[1] );
 
-      var message = msgpack.decode( client.messages[2] );
       assert.equal(message[0], Protocol.ROOM_STATE);
       assert.deepEqual(message[2], { success: true });
     });
@@ -135,6 +130,14 @@ describe('Room', function() {
       // set state
       room.setState({one: 1});
       assert.deepEqual({one: 1}, room.state);
+
+      // clean state. no patches available!
+      assert.equal(false, (<any>room).broadcastPatch());
+
+      // change the state to make patch available
+      room.state.one = 111;
+
+      // voila!
       assert.equal(true, (<any>room).broadcastPatch());
     });
 
@@ -150,14 +153,19 @@ describe('Room', function() {
       (<any>room)._onJoin(client2, {});
 
       assert.deepEqual({one: 1}, room.state);
-      assert.equal(true, (<any>room).broadcastPatch());
 
+      // clean state. no patches available!
+      assert.equal(false, (<any>room).broadcastPatch());
+
+      // change the state to make patch available
       room.state.two = 2;
       assert.deepEqual({one: 1, two: 2}, room.state);
+
+      // voila!
       assert.equal(true, (<any>room).broadcastPatch());
 
-      assert.equal(client.messages.length, 4);
-      assert.equal(client2.messages.length, 4);
+      assert.equal(client.messages.length, 3);
+      assert.equal(client2.messages.length, 3);
 
       // first message, join room
       var message = msgpack.decode(client.messages[0]);
@@ -170,13 +178,7 @@ describe('Room', function() {
       // third message, empty patch state
       var message = msgpack.decode(client.messages[2]);
       assert.equal(message[0], Protocol.ROOM_STATE_PATCH);
-      assert.deepEqual(message[2].length, 17);
-      // // TODO: ideally empty patch state should have 0 length
-      // assert.deepEqual(message[2].length, 0);
-
-      // fourth message, room patch state
-      var message = msgpack.decode(client.messages[3]);
-      assert.equal(message[0], Protocol.ROOM_STATE_PATCH);
+      assert.deepEqual(message[2].length, 22);
 
       assert.deepEqual(message[2], [ 66, 10, 66, 58, 130, 163, 111, 110, 101, 1, 163, 116, 119, 111, 2, 49, 86, 53, 49, 74, 89, 59 ]);
     });

diff --git a/yarn.lock b/yarn.lock
@@ -1854,10 +1854,6 @@ uid-number@~0.0.6:
   version "0.0.6"
   resolved "https://registry.yarnpkg.com/uid-number/-/uid-number-0.0.6.tgz#0ea10e8035e8eb5b8e4449f06da1c730663baa81"
 
-ultron@~1.1.0:
-  version "1.1.0"
-  resolved "https://registry.yarnpkg.com/ultron/-/ultron-1.1.0.tgz#b07a2e6a541a815fc6a34ccd4533baec307ca864"
-
 undefsafe@0.0.3:
   version "0.0.3"
   resolved "https://registry.yarnpkg.com/undefsafe/-/undefsafe-0.0.3.tgz#ecca3a03e56b9af17385baac812ac83b994a962f"
@@ -1963,12 +1959,6 @@ write-file-atomic@^1.1.2:
     imurmurhash "^0.1.4"
     slide "^1.1.5"
 
-ws@^2.0.0:
-  version "2.0.1"
-  resolved "https://registry.yarnpkg.com/ws/-/ws-2.0.1.tgz#0d3498dcb29dbee9fa229e61ebffeba67316a827"
-  dependencies:
-    ultron "~1.1.0"
-
 xdg-basedir@^2.0.0:
   version "2.0.0"
   resolved "https://registry.yarnpkg.com/xdg-basedir/-/xdg-basedir-2.0.0.tgz#edbc903cc385fc04523d966a335504b5504d1bd2"