Repository: websockets/ws
Pull request #914 (2016-11-26 17:44:45)

Simplify closing handshake
Currently the close handshake works like this when initiated from the server:

```
        SERVER                                 CLIENT


+--------------------+
|ws.close()          |
|readyState = CLOSING|
+--------------------+
           |            +-----------+
           +----------->|close frame|-------------+
                        +-----------+             v
                                       +--------------------+
                                       |closeReceived = true|
                                       |ws.close()          |
                                       |readyState = CLOSING|
                                       +--------------------+
                        +-----------+             |
           +------------|close frame|<------------+
           v            +-----------+
+--------------------+
|closeReceived = true|
|ws.close()          |
|ws.terminate()      |
|socket.end()        |
+--------------------+
           |              +-------+
           +------------->|TCP FIN|---------------+
                          +-------+               v
                                        +-------------------+
                                        |readyState = CLOSED|
                                        |ws.emit('closed')  |
                                        |socket.end()       |
                                        +-------------------+
                          +-------+               |
           +--------------|TCP FIN|<--------------+
           |              +-------+
           v
 +-------------------+
 |readyState = CLOSED|
 |ws.emit('closed')  |
 +-------------------+
```

and like this when initiated from the client:

```
        CLIENT                                 SERVER


+--------------------+
|ws.close()          |
|readyState = CLOSING|
+--------------------+
           |            +-----------+
           +----------->|close frame|-------------+
                        +-----------+             v
                                       +--------------------+
                                       |closeReceived = true|
                                       |ws.close()          |
                                       |readyState = CLOSING|
                                       |ws.terminate()      |
                                       |socket.end()        |
                                       +--------------------+
                   +---------------------+        |
           +-------|close frame + TCP FIN|<-------+
           v       +---------------------+
+--------------------+
|closeReceived = true|
|ws.close()          |
|readyState = CLOSED |
|ws.emit('closed')   |
|socket.end()        |
+--------------------+
           |              +-------+
           +------------->|TCP FIN|---------------+
                          +-------+               v
                                        +-------------------+
                                        |readyState = CLOSED|
                                        |ws.emit('closed')  |
                                        +-------------------+
```

This patch uniforms the behavior making a close initiated by the server work in the same way of a close initiated by the client.

After reading https://tools.ietf.org/html/rfc6455#section-1.4 and https://tools.ietf.org/html/rfc6455#section-7, I think this is correct.

I have some doubts on this [section](https://tools.ietf.org/html/rfc6455#section-7.1.1) which is why the close handshake is currently implemented in this way for the server but I also checked other implementations for Node.js and they use the same procedure for both client and server as proposed in this patch.

lpinca: cc: @nkzawa 
lpinca: I guess the question to answer is:

Is there any advantage on calling `socket.end()` first on the client and then on the server when a close procedure is initiated by the server?

I don't know much about low level networking so if someone could shed some light on this, it would be great.

Patch:
diff --git a/bench/parser.benchmark.js b/bench/parser.benchmark.js
index 63883f8e..2924b544 100644
--- a/bench/parser.benchmark.js
+++ b/bench/parser.benchmark.js
@@ -43,8 +43,10 @@ const binaryDataPacket2 = createBinaryPacket(65535);
 const binaryDataPacket3 = createBinaryPacket(200 * 1024);
 const binaryDataPacket4 = createBinaryPacket(1024 * 1024);
 
-const receiver = new Receiver();
 const suite = new benchmark.Suite();
+const receiver = new Receiver();
+
+receiver.onmessage = receiver.onclose = receiver.onping = () => {};
 
 suite.add('ping message', () => receiver.add(pingPacket1));
 suite.add('ping with no data', () => receiver.add(pingPacket2));
diff --git a/lib/Receiver.js b/lib/Receiver.js
index 07cbb9b6..07aa0596 100644
--- a/lib/Receiver.js
+++ b/lib/Receiver.js
@@ -20,8 +20,6 @@ const GET_MASK = 3;
 const GET_DATA = 4;
 const HANDLE_DATA = 5;
 
-const noop = () => {};
-
 /**
  * HyBi Receiver implementation.
  */
@@ -53,11 +51,11 @@ class Receiver {
 
     this.dead = false;
 
-    this.onmessage = noop;
-    this.onclose = noop;
-    this.onerror = noop;
-    this.onping = noop;
-    this.onpong = noop;
+    this.onmessage = null;
+    this.onclose = null;
+    this.onerror = null;
+    this.onping = null;
+    this.onpong = null;
 
     this.state = START;
   }
@@ -69,19 +67,17 @@ class Receiver {
    * @private
    */
   readBuffer (bytes) {
-    var bufoff = 0;
+    var offset = 0;
     var dst;
     var l;
 
-    if (bytes === this.buffers[0].length) {
-      this.bufferedBytes -= bytes;
-      return this.buffers.shift();
-    }
+    this.bufferedBytes -= bytes;
+
+    if (bytes === this.buffers[0].length) return this.buffers.shift();
 
     if (bytes < this.buffers[0].length) {
       dst = this.buffers[0].slice(0, bytes);
       this.buffers[0] = this.buffers[0].slice(bytes);
-      this.bufferedBytes -= bytes;
       return dst;
     }
 
@@ -90,15 +86,13 @@ class Receiver {
     while (bytes > 0) {
       l = this.buffers[0].length;
 
-      if (bytes > l) {
-        this.buffers[0].copy(dst, bufoff);
-        bufoff += l;
+      if (bytes >= l) {
+        this.buffers[0].copy(dst, offset);
+        offset += l;
         this.buffers.shift();
-        this.bufferedBytes -= l;
       } else {
-        this.buffers[0].copy(dst, bufoff, 0, bytes);
+        this.buffers[0].copy(dst, offset, 0, bytes);
         this.buffers[0] = this.buffers[0].slice(bytes);
-        this.bufferedBytes -= bytes;
       }
 
       bytes -= l;
diff --git a/lib/WebSocket.js b/lib/WebSocket.js
index 98b5eeec..ecede686 100644
--- a/lib/WebSocket.js
+++ b/lib/WebSocket.js
@@ -255,9 +255,7 @@ class WebSocket extends EventEmitter {
     }
 
     if (this.readyState === WebSocket.CLOSING) {
-      if (this._closeCode && this._isServer) {
-        this.terminate();
-      }
+      if (this._closeCode) this.terminate();
       return;
     }
 
@@ -265,7 +263,7 @@ class WebSocket extends EventEmitter {
     this._sender.close(code, data, !this._isServer, (err) => {
       if (err) this.emit('error', err);
 
-      if (this._closeCode && this._isServer) {
+      if (this._closeCode) {
         this.terminate();
       } else {
         //


Test patch:
diff --git a/test/Receiver.test.js b/test/Receiver.test.js
index 18dc711a..1ee811ce 100644
--- a/test/Receiver.test.js
+++ b/test/Receiver.test.js
@@ -309,39 +309,50 @@ describe('Receiver', function () {
 
   it('resets `totalPayloadLength` only on final frame (unfragmented)', function () {
     const p = new Receiver({}, 10);
+    let message;
+
+    p.onmessage = function (msg) {
+      message = msg;
+    };
 
     assert.strictEqual(p.totalPayloadLength, 0);
     p.add(Buffer.from('810548656c6c6f', 'hex'));
     assert.strictEqual(p.totalPayloadLength, 0);
+    assert.strictEqual(message, 'Hello');
   });
 
   it('resets `totalPayloadLength` only on final frame (fragmented)', function () {
     const p = new Receiver({}, 10);
+    let message;
 
-    const frame1 = '01024865';
-    const frame2 = '80036c6c6f';
+    p.onmessage = function (msg) {
+      message = msg;
+    };
 
     assert.strictEqual(p.totalPayloadLength, 0);
-    p.add(Buffer.from(frame1, 'hex'));
+    p.add(Buffer.from('01024865', 'hex'));
     assert.strictEqual(p.totalPayloadLength, 2);
-    p.add(Buffer.from(frame2, 'hex'));
+    p.add(Buffer.from('80036c6c6f', 'hex'));
     assert.strictEqual(p.totalPayloadLength, 0);
+    assert.strictEqual(message, 'Hello');
   });
 
   it('resets `totalPayloadLength` only on final frame (fragmented + ping)', function () {
     const p = new Receiver({}, 10);
+    const data = [];
 
-    const frame1 = '01024865';
-    const frame2 = '8900';
-    const frame3 = '80036c6c6f';
+    p.onmessage = p.onping = function (buf) {
+      data.push(buf.toString());
+    };
 
     assert.strictEqual(p.totalPayloadLength, 0);
-    p.add(Buffer.from(frame1, 'hex'));
+    p.add(Buffer.from('02024865', 'hex'));
     assert.strictEqual(p.totalPayloadLength, 2);
-    p.add(Buffer.from(frame2, 'hex'));
+    p.add(Buffer.from('8900', 'hex'));
     assert.strictEqual(p.totalPayloadLength, 2);
-    p.add(Buffer.from(frame3, 'hex'));
+    p.add(Buffer.from('80036c6c6f', 'hex'));
     assert.strictEqual(p.totalPayloadLength, 0);
+    assert.deepStrictEqual(data, ['', 'Hello']);
   });
 
   it('raises an error when RSV1 is on and permessage-deflate is disabled', function (done) {
diff --git a/test/WebSocket.test.js b/test/WebSocket.test.js
index 24732207..cc700431 100644
--- a/test/WebSocket.test.js
+++ b/test/WebSocket.test.js
@@ -1010,6 +1010,39 @@ describe('WebSocket', function () {
 
       wss.on('connection', (ws) => ws.close(1013));
     });
+
+    it('closes the connection when an error occurs', function (done) {
+      const server = http.createServer();
+      const wss = new WebSocketServer({ server });
+      let closed = false;
+
+      wss.on('connection', (ws) => {
+        ws.on('error', (err) => {
+          assert.ok(err instanceof Error);
+          assert.strictEqual(err.message, 'RSV2 and RSV3 must be clear');
+
+          ws.on('close', (code, reason) => {
+            assert.strictEqual(code, 1006);
+            assert.strictEqual(reason, '');
+
+            closed = true;
+          });
+        });
+      });
+
+      server.listen(++port, () => {
+        const ws = new WebSocket(`ws://localhost:${port}`);
+
+        ws.on('open', () => ws._socket.write(Buffer.from([0xa1, 0x00])));
+        ws.on('close', (code, reason) => {
+          assert.strictEqual(code, 1002);
+          assert.strictEqual(reason, '');
+          assert.ok(closed);
+
+          server.close(done);
+        });
+      });
+    });
   });
 
   describe('WHATWG API emulation', function () {