Repository: websockets/ws
Pull request #1016 (2017-02-23 17:12:41)

Make `Sender#frameAndSend()` a static method
This transforms `Sender.prototype.frameAndSend()` into a static method (`Sender.frame()`) which can be used to frame data.

Having a function which returns a frame gives developers more fine grained control and allows them to optimize certain operations like a server broadcast where the same frame should be sent to all clients.

guymguym: LGTM

Patch:
diff --git a/bench/sender.benchmark.js b/bench/sender.benchmark.js
index a0f19a87..46132c19 100644
--- a/bench/sender.benchmark.js
+++ b/bench/sender.benchmark.js
@@ -33,19 +33,17 @@ const opts2 = {
 };
 
 const suite = new benchmark.Suite();
-var sender = new Sender();
-sender._socket = { write () {} };
-
-suite.add('frameAndSend, unmasked (64 B)', () => sender.frameAndSend(data1, opts1));
-suite.add('frameAndSend, masked (64 B)', () => sender.frameAndSend(data1, opts2));
-suite.add('frameAndSend, unmasked (16 KiB)', () => sender.frameAndSend(data2, opts1));
-suite.add('frameAndSend, masked (16 KiB)', () => sender.frameAndSend(data2, opts2));
-suite.add('frameAndSend, unmasked (64 KiB)', () => sender.frameAndSend(data3, opts1));
-suite.add('frameAndSend, masked (64 KiB)', () => sender.frameAndSend(data3, opts2));
-suite.add('frameAndSend, unmasked (200 KiB)', () => sender.frameAndSend(data4, opts1));
-suite.add('frameAndSend, masked (200 KiB)', () => sender.frameAndSend(data4, opts2));
-suite.add('frameAndSend, unmasked (1 MiB)', () => sender.frameAndSend(data5, opts1));
-suite.add('frameAndSend, masked (1 MiB)', () => sender.frameAndSend(data5, opts2));
+
+suite.add('frame, unmasked (64 B)', () => Sender.frame(data1, opts1));
+suite.add('frame, masked (64 B)', () => Sender.frame(data1, opts2));
+suite.add('frame, unmasked (16 KiB)', () => Sender.frame(data2, opts1));
+suite.add('frame, masked (16 KiB)', () => Sender.frame(data2, opts2));
+suite.add('frame, unmasked (64 KiB)', () => Sender.frame(data3, opts1));
+suite.add('frame, masked (64 KiB)', () => Sender.frame(data3, opts2));
+suite.add('frame, unmasked (200 KiB)', () => Sender.frame(data4, opts1));
+suite.add('frame, masked (200 KiB)', () => Sender.frame(data4, opts2));
+suite.add('frame, unmasked (1 MiB)', () => Sender.frame(data5, opts1));
+suite.add('frame, masked (1 MiB)', () => Sender.frame(data5, opts2));
 
 suite.on('cycle', (e) => console.log(e.target.toString()));
 
diff --git a/lib/Sender.js b/lib/Sender.js
index b3e6cd73..d36c7ba5 100644
--- a/lib/Sender.js
+++ b/lib/Sender.js
@@ -36,6 +36,71 @@ class Sender {
     this.onerror = null;
   }
 
+  /**
+   * Frames a piece of data according to the HyBi WebSocket protocol.
+   *
+   * @param {Buffer} data The data to frame
+   * @param {Object} options Options object
+   * @param {Number} options.opcode The opcode
+   * @param {Boolean} options.readOnly Specifies whether `data` can be modified
+   * @param {Boolean} options.fin Specifies whether or not to set the FIN bit
+   * @param {Boolean} options.mask Specifies whether or not to mask `data`
+   * @param {Boolean} options.rsv1 Specifies whether or not to set the RSV1 bit
+   * @return {Buffer[]} The framed data as a list of `Buffer` instances
+   * @public
+   */
+  static frame (data, options) {
+    const merge = data.length < 1024 || options.mask && options.readOnly;
+    var offset = options.mask ? 6 : 2;
+    var payloadLength = data.length;
+
+    if (data.length >= 65536) {
+      offset += 8;
+      payloadLength = 127;
+    } else if (data.length > 125) {
+      offset += 2;
+      payloadLength = 126;
+    }
+
+    const target = Buffer.allocUnsafe(merge ? data.length + offset : offset);
+
+    target[0] = options.fin ? options.opcode | 0x80 : options.opcode;
+    if (options.rsv1) target[0] |= 0x40;
+
+    if (payloadLength === 126) {
+      target.writeUInt16BE(data.length, 2, true);
+    } else if (payloadLength === 127) {
+      target.writeUInt32BE(0, 2, true);
+      target.writeUInt32BE(data.length, 6, true);
+    }
+
+    if (!options.mask) {
+      target[1] = payloadLength;
+      if (merge) {
+        data.copy(target, offset);
+        return [target];
+      }
+
+      return [target, data];
+    }
+
+    const mask = crypto.randomBytes(4);
+
+    target[1] = payloadLength | 0x80;
+    target[offset - 4] = mask[0];
+    target[offset - 3] = mask[1];
+    target[offset - 2] = mask[2];
+    target[offset - 1] = mask[3];
+
+    if (merge) {
+      bufferUtil.mask(data, mask, target, offset, data.length);
+      return [target];
+    }
+
+    bufferUtil.mask(data, mask, data, 0, data.length);
+    return [target, data];
+  }
+
   /**
    * Sends a close message to the other peer.
    *
@@ -71,13 +136,13 @@ class Sender {
    * @private
    */
   doClose (data, mask, cb) {
-    this.frameAndSend(data, {
+    this.sendFrame(Sender.frame(data, {
       readOnly: false,
       opcode: 0x08,
       rsv1: false,
       fin: true,
       mask
-    }, cb);
+    }), cb);
   }
 
   /**
@@ -117,13 +182,13 @@ class Sender {
    * @private
    */
   doPing (data, mask, readOnly) {
-    this.frameAndSend(data, {
+    this.sendFrame(Sender.frame(data, {
       opcode: 0x09,
       rsv1: false,
       fin: true,
       readOnly,
       mask
-    });
+    }));
   }
 
   /**
@@ -163,13 +228,13 @@ class Sender {
    * @private
    */
   doPong (data, mask, readOnly) {
-    this.frameAndSend(data, {
+    this.sendFrame(Sender.frame(data, {
       opcode: 0x0a,
       rsv1: false,
       fin: true,
       readOnly,
       mask
-    });
+    }));
   }
 
   /**
@@ -229,13 +294,13 @@ class Sender {
         this.dispatch(data, opts, cb);
       }
     } else {
-      this.frameAndSend(data, {
+      this.sendFrame(Sender.frame(data, {
         mask: options.mask,
         fin: options.fin,
         rsv1: false,
         readOnly,
         opcode
-      }, cb);
+      }), cb);
     }
   }
 
@@ -255,7 +320,7 @@ class Sender {
    */
   dispatch (data, options, cb) {
     if (!options.compress) {
-      this.frameAndSend(data, options, cb);
+      this.sendFrame(Sender.frame(data, options), cb);
       return;
     }
 
@@ -268,74 +333,12 @@ class Sender {
       }
 
       options.readOnly = false;
-      this.frameAndSend(buf, options, cb);
+      this.sendFrame(Sender.frame(buf, options), cb);
       this.deflating = false;
       this.dequeue();
     });
   }
 
-  /**
-   * Frames and sends a piece of data according to the HyBi WebSocket protocol.
-   *
-   * @param {Buffer} data The data to send
-   * @param {Object} options Options object
-   * @param {Number} options.opcode The opcode
-   * @param {Boolean} options.readOnly Specifies whether `data` can be modified
-   * @param {Boolean} options.fin Specifies whether or not to set the FIN bit
-   * @param {Boolean} options.mask Specifies whether or not to mask `data`
-   * @param {Boolean} options.rsv1 Specifies whether or not to set the RSV1 bit
-   * @param {Function} cb Callback
-   * @private
-   */
-  frameAndSend (data, options, cb) {
-    const mergeBuffers = data.length < 1024 || options.mask && options.readOnly;
-    var dataOffset = options.mask ? 6 : 2;
-    var payloadLength = data.length;
-
-    if (data.length >= 65536) {
-      dataOffset += 8;
-      payloadLength = 127;
-    } else if (data.length > 125) {
-      dataOffset += 2;
-      payloadLength = 126;
-    }
-
-    const outputBuffer = Buffer.allocUnsafe(
-      mergeBuffers ? data.length + dataOffset : dataOffset
-    );
-
-    outputBuffer[0] = options.fin ? options.opcode | 0x80 : options.opcode;
-    if (options.rsv1) outputBuffer[0] |= 0x40;
-
-    if (payloadLength === 126) {
-      outputBuffer.writeUInt16BE(data.length, 2, true);
-    } else if (payloadLength === 127) {
-      outputBuffer.writeUInt32BE(0, 2, true);
-      outputBuffer.writeUInt32BE(data.length, 6, true);
-    }
-
-    if (options.mask) {
-      const mask = getRandomMask();
-
-      outputBuffer[1] = payloadLength | 0x80;
-      outputBuffer[dataOffset - 4] = mask[0];
-      outputBuffer[dataOffset - 3] = mask[1];
-      outputBuffer[dataOffset - 2] = mask[2];
-      outputBuffer[dataOffset - 1] = mask[3];
-
-      if (mergeBuffers) {
-        bufferUtil.mask(data, mask, outputBuffer, dataOffset, data.length);
-      } else {
-        bufferUtil.mask(data, mask, data, 0, data.length);
-      }
-    } else {
-      outputBuffer[1] = payloadLength;
-      if (mergeBuffers) data.copy(outputBuffer, dataOffset);
-    }
-
-    sendFramedData(this, outputBuffer, mergeBuffers ? null : data, cb);
-  }
-
   /**
    * Executes queued send operations.
    *
@@ -360,6 +363,22 @@ class Sender {
     this.bufferedBytes += params[1].length;
     this.queue.push(params);
   }
+
+  /**
+   * Sends a frame.
+   *
+   * @param {Buffer[]} list The frame to send
+   * @param {Function} cb Callback
+   * @private
+   */
+  sendFrame (list, cb) {
+    if (list.length === 2) {
+      this._socket.write(list[0]);
+      this._socket.write(list[1], cb);
+    } else {
+      this._socket.write(list[0], cb);
+    }
+  }
 }
 
 module.exports = Sender;
@@ -380,31 +399,3 @@ function viewToBuffer (view) {
 
   return buf;
 }
-
-/**
- * Generates a random mask.
- *
- * @return {Buffer} The mask
- * @private
- */
-function getRandomMask () {
-  return crypto.randomBytes(4);
-}
-
-/**
- * Sends a frame.
- *
- * @param {Sender} sender Sender instance
- * @param {Buffer} outputBuffer The data to send
- * @param {Buffer} data Additional data to send if frame is split into two buffers
- * @param {Function} cb Callback
- * @private
- */
-function sendFramedData (sender, outputBuffer, data, cb) {
-  if (data) {
-    sender._socket.write(outputBuffer);
-    sender._socket.write(data, cb);
-  } else {
-    sender._socket.write(outputBuffer, cb);
-  }
-}


Test patch:
diff --git a/test/Sender.test.js b/test/Sender.test.js
index da511051..7c60c1e8 100644
--- a/test/Sender.test.js
+++ b/test/Sender.test.js
@@ -6,12 +6,11 @@ const PerMessageDeflate = require('../lib/PerMessageDeflate');
 const Sender = require('../lib/Sender');
 
 describe('Sender', function () {
-  describe('#frameAndSend', function () {
-    it('does not modify a masked binary buffer', function () {
-      const sender = new Sender({ write: () => {} });
+  describe('.frame', function () {
+    it('does not mutate the input buffer if data is `readOnly`', function () {
       const buf = Buffer.from([1, 2, 3, 4, 5]);
 
-      sender.frameAndSend(buf, {
+      Sender.frame(buf, {
         readOnly: true,
         rsv1: false,
         mask: true,
@@ -22,36 +21,16 @@ describe('Sender', function () {
       assert.ok(buf.equals(Buffer.from([1, 2, 3, 4, 5])));
     });
 
-    it('does not modify a masked text buffer', function () {
-      const sender = new Sender({ write: () => {} });
-      const text = Buffer.from('hi there');
-
-      sender.frameAndSend(text, {
-        readOnly: true,
-        rsv1: false,
-        mask: true,
-        opcode: 1,
-        fin: true
-      });
-
-      assert.ok(text.equals(Buffer.from('hi there')));
-    });
-
-    it('sets RSV1 bit if compressed', function (done) {
-      const sender = new Sender({
-        write: (data) => {
-          assert.strictEqual(data[0] & 0x40, 0x40);
-          done();
-        }
-      });
-
-      sender.frameAndSend(Buffer.from('hi'), {
+    it('sets RSV1 bit if compressed', function () {
+      const list = Sender.frame(Buffer.from('hi'), {
         readOnly: false,
         mask: false,
         rsv1: true,
         opcode: 1,
         fin: true
       });
+
+      assert.strictEqual(list[0][0] & 0x40, 0x40);
     });
   });