Skip to content

Commit

Permalink
[major] Do not decode Buffers to strings
Browse files Browse the repository at this point in the history
Avoid decoding text messages and close reasons to strings. Pass them as
`Buffer`s to the listeners of their respective events. Also, make
listeners of the `'message'` event take a boolean argument to speficy
whether or not the message is binary.

Refs: #1878
Refs: #1804
  • Loading branch information
lpinca committed Jul 14, 2021
1 parent ebea038 commit e173423
Show file tree
Hide file tree
Showing 15 changed files with 326 additions and 209 deletions.
12 changes: 6 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,8 @@ ws.on('open', function open() {
ws.send('something');
});

ws.on('message', function incoming(data) {
console.log(data);
ws.on('message', function incoming(message) {
console.log('received: %s', message);
});
```

Expand Down Expand Up @@ -296,10 +296,10 @@ const WebSocket = require('ws');
const wss = new WebSocket.Server({ port: 8080 });

wss.on('connection', function connection(ws) {
ws.on('message', function incoming(data) {
ws.on('message', function incoming(data, isBinary) {
wss.clients.forEach(function each(client) {
if (client.readyState === WebSocket.OPEN) {
client.send(data);
client.send(data, { binary: isBinary });
}
});
});
Expand All @@ -315,10 +315,10 @@ const WebSocket = require('ws');
const wss = new WebSocket.Server({ port: 8080 });

wss.on('connection', function connection(ws) {
ws.on('message', function incoming(data) {
ws.on('message', function incoming(data, isBinary) {
wss.clients.forEach(function each(client) {
if (client !== ws && client.readyState === WebSocket.OPEN) {
client.send(data);
client.send(data, { binary: isBinary });
}
});
});
Expand Down
4 changes: 3 additions & 1 deletion bench/speed.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ if (cluster.isMaster) {
});

wss.on('connection', (ws) => {
ws.on('message', (data) => ws.send(data));
ws.on('message', (data, isBinary) => {
ws.send(data, { binary: isBinary });
});
});

server.listen(path ? { path } : { port }, () => cluster.fork());
Expand Down
14 changes: 8 additions & 6 deletions doc/ws.md
Original file line number Diff line number Diff line change
Expand Up @@ -300,11 +300,12 @@ it defaults to `/`.
### Event: 'close'

- `code` {Number}
- `reason` {String}
- `reason` {Buffer}

Emitted when the connection is closed. `code` is a numeric value indicating the
status code explaining why the connection has been closed. `reason` is a
human-readable string explaining why the connection has been closed.
`Buffer` containing a human-readable string explaining why the connection has
been closed.

### Event: 'error'

Expand All @@ -315,9 +316,11 @@ of the string values defined below under [WS Error Codes](#ws-error-codes).

### Event: 'message'

- `data` {String|Buffer|ArrayBuffer|Buffer[]}
- `data` {Buffer|ArrayBuffer|Buffer[]}
- `isBinary` {Boolean}

Emitted when a message is received from the server.
Emitted when a message is received. `data` is the message content. `isBinary`
specifies whether the message is binary or not.

### Event: 'open'

Expand Down Expand Up @@ -389,8 +392,7 @@ following ways:

- `code` {Number} A numeric value indicating the status code explaining why the
connection is being closed.
- `reason` {String} A human-readable string explaining why the connection is
closing.
- `reason` {String|Buffer} The reason why the connection is closing.

Initiate a closing handshake.

Expand Down
2 changes: 1 addition & 1 deletion examples/ssl.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const wss = new WebSocket.Server({ server });

wss.on('connection', function connection(ws) {
ws.on('message', function message(msg) {
console.log(msg);
console.log(msg.toString());
});
});

Expand Down
9 changes: 6 additions & 3 deletions lib/event-target.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,12 +128,15 @@ const EventTarget = {
addEventListener(type, listener, options) {
if (typeof listener !== 'function') return;

function onMessage(data) {
listener.call(this, new MessageEvent(data, this));
function onMessage(data, isBinary) {
listener.call(
this,
new MessageEvent(isBinary ? data : data.toString(), this)
);
}

function onClose(code, message) {
listener.call(this, new CloseEvent(code, message, this));
listener.call(this, new CloseEvent(code, message.toString(), this));
}

function onError(error) {
Expand Down
8 changes: 4 additions & 4 deletions lib/receiver.js
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,7 @@ class Receiver extends Writable {
data = fragments;
}

this.emit('message', data);
this.emit('message', data, true);
} else {
const buf = concat(fragments, messageLength);

Expand All @@ -514,7 +514,7 @@ class Receiver extends Writable {
);
}

this.emit('message', buf.toString());
this.emit('message', buf, false);
}
}

Expand All @@ -533,7 +533,7 @@ class Receiver extends Writable {
this._loop = false;

if (data.length === 0) {
this.emit('conclude', 1005, '');
this.emit('conclude', 1005, EMPTY_BUFFER);
this.end();
} else if (data.length === 1) {
return error(
Expand Down Expand Up @@ -568,7 +568,7 @@ class Receiver extends Writable {
);
}

this.emit('conclude', code, buf.toString());
this.emit('conclude', code, buf);
this.end();
}
} else if (this._opcode === 0x09) {
Expand Down
11 changes: 8 additions & 3 deletions lib/sender.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ class Sender {
* Sends a close message to the other peer.
*
* @param {Number} [code] The status code component of the body
* @param {String} [data] The message component of the body
* @param {(String|Buffer)} [data] The message component of the body
* @param {Boolean} [mask=false] Specifies whether or not to mask the message
* @param {Function} [cb] Callback
* @public
Expand All @@ -114,7 +114,7 @@ class Sender {
buf = EMPTY_BUFFER;
} else if (typeof code !== 'number' || !isValidStatusCode(code)) {
throw new TypeError('First argument must be a valid error code number');
} else if (data === undefined || data === '') {
} else if (data === undefined || !data.length) {
buf = Buffer.allocUnsafe(2);
buf.writeUInt16BE(code, 0);
} else {
Expand All @@ -126,7 +126,12 @@ class Sender {

buf = Buffer.allocUnsafe(2 + length);
buf.writeUInt16BE(code, 0);
buf.write(data, 2);

if (typeof data === 'string') {
buf.write(data, 2);
} else {
buf.set(data, 2);
}
}

if (this._deflating) {
Expand Down
7 changes: 5 additions & 2 deletions lib/stream.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,11 @@ function createWebSocketStream(ws, options) {
writableObjectMode: false
});

ws.on('message', function message(msg) {
if (!duplex.push(msg)) {
ws.on('message', function message(msg, isBinary) {
const data =
!isBinary && duplex._readableState.objectMode ? msg.toString() : msg;

if (!duplex.push(data)) {
resumeOnReceiverDrain = false;
ws._socket.pause();
}
Expand Down
14 changes: 8 additions & 6 deletions lib/websocket.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class WebSocket extends EventEmitter {
this._closeCode = 1006;
this._closeFrameReceived = false;
this._closeFrameSent = false;
this._closeMessage = '';
this._closeMessage = EMPTY_BUFFER;
this._closeTimer = null;
this._extensions = {};
this._protocol = '';
Expand Down Expand Up @@ -264,7 +264,8 @@ class WebSocket extends EventEmitter {
* +---+
*
* @param {Number} [code] Status code explaining why the connection is closing
* @param {String} [data] A string explaining why the connection is closing
* @param {(String|Buffer)} [data] The reason why the connection is
* closing
* @public
*/
close(code, data) {
Expand Down Expand Up @@ -941,7 +942,7 @@ function sendAfterClose(websocket, data, cb) {
* The listener of the `Receiver` `'conclude'` event.
*
* @param {Number} code The status code
* @param {String} reason The reason for closing
* @param {Buffer} reason The reason for closing
* @private
*/
function receiverOnConclude(code, reason) {
Expand Down Expand Up @@ -995,11 +996,12 @@ function receiverOnFinish() {
/**
* The listener of the `Receiver` `'message'` event.
*
* @param {(String|Buffer|ArrayBuffer|Buffer[])} data The message
* @param {Buffer|ArrayBuffer|Buffer[])} data The message
* @param {Boolean} isBinary Specifies whether the message is binary or not
* @private
*/
function receiverOnMessage(data) {
this[kWebSocket].emit('message', data);
function receiverOnMessage(data, isBinary) {
this[kWebSocket].emit('message', data, isBinary);
}

/**
Expand Down
4 changes: 3 additions & 1 deletion test/autobahn-server.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ const wss = new WebSocket.Server({ port }, () => {
});

wss.on('connection', (ws) => {
ws.on('message', (data) => ws.send(data));
ws.on('message', (data, isBinary) => {
ws.send(data, { binary: isBinary });
});
ws.on('error', (e) => console.error(e));
});
4 changes: 3 additions & 1 deletion test/autobahn.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ function nextTest() {
ws = new WebSocket(
`ws://localhost:9001/runCase?case=${currentTest}&agent=ws`
);
ws.on('message', (data) => ws.send(data));
ws.on('message', (data, isBinary) => {
ws.send(data, { binary: isBinary });
});
ws.on('close', () => {
currentTest++;
process.nextTick(nextTest);
Expand Down
38 changes: 34 additions & 4 deletions test/create-websocket-stream.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const { randomBytes } = require('crypto');
const createWebSocketStream = require('../lib/stream');
const Sender = require('../lib/sender');
const WebSocket = require('..');
const { EMPTY_BUFFER } = require('../lib/constants');

describe('createWebSocketStream', () => {
it('is exposed as a property of the `WebSocket` class', () => {
Expand Down Expand Up @@ -58,11 +59,12 @@ describe('createWebSocketStream', () => {
});

wss.on('connection', (ws) => {
ws.on('message', (message) => {
ws.on('message', (message, isBinary) => {
ws.on('close', (code, reason) => {
assert.ok(message.equals(chunk));
assert.deepStrictEqual(message, chunk);
assert.ok(isBinary);
assert.strictEqual(code, 1005);
assert.strictEqual(reason, '');
assert.strictEqual(reason, EMPTY_BUFFER);
wss.close(done);
});
});
Expand Down Expand Up @@ -229,7 +231,7 @@ describe('createWebSocketStream', () => {
ws._socket.write(Buffer.from([0x85, 0x00]));
ws.on('close', (code, reason) => {
assert.strictEqual(code, 1002);
assert.strictEqual(reason, '');
assert.deepStrictEqual(reason, EMPTY_BUFFER);

serverClientCloseEventEmitted = true;
if (duplexCloseEventEmitted) wss.close(done);
Expand Down Expand Up @@ -538,5 +540,33 @@ describe('createWebSocketStream', () => {
});
});
});

it('converts text messages to strings in readable object mode', (done) => {
const wss = new WebSocket.Server({ port: 0 }, () => {
const events = [];
const ws = new WebSocket(`ws://localhost:${wss.address().port}`);
const duplex = createWebSocketStream(ws, { readableObjectMode: true });

duplex.on('data', (data) => {
events.push('data');
assert.strictEqual(data, 'foo');
});

duplex.on('end', () => {
events.push('end');
duplex.end();
});

duplex.on('close', () => {
assert.deepStrictEqual(events, ['data', 'end']);
wss.close(done);
});
});

wss.on('connection', (ws) => {
ws.send('foo');
ws.close();
});
});
});
});

0 comments on commit e173423

Please sign in to comment.