Skip to content

Commit

Permalink
[fix] Don't call ws.terminate() unconditionally in duplex._destroy()
Browse files Browse the repository at this point in the history
Call `ws.terminate()` only if `duplex.destroy()` is called directly by
the user and not indirectly by the listener of the `'error'` event of
the `WebSocket` object. Calling `ws.terminate()` right after the
`'error'` event is emitted on the `WebSocket` object, might prevent the
close frame from being sent to the other peer.
  • Loading branch information
lpinca committed Jun 9, 2021
1 parent 8806aa9 commit 074e6a8
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 2 deletions.
14 changes: 13 additions & 1 deletion lib/stream.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ function duplexOnError(err) {
*/
function createWebSocketStream(ws, options) {
let resumeOnReceiverDrain = true;
let terminateOnDestroy = true;

function receiverOnDrain() {
if (resumeOnReceiverDrain) ws._socket.resume();
Expand Down Expand Up @@ -81,6 +82,16 @@ function createWebSocketStream(ws, options) {
ws.once('error', function error(err) {
if (duplex.destroyed) return;

// Prevent `ws.terminate()` from being called by `duplex._destroy()`.
//
// - If the state of the `WebSocket` connection is `CONNECTING`,
// `ws.terminate()` is a noop as no socket was assigned.
// - Otherwise, the error was re-emitted from the listener of the `'error'`
// event of the `Receiver` object. The listener already closes the
// connection by calling `ws.close()`. This allows a close frame to be
// sent to the other peer. If `ws.terminate()` is called right after this,
// the close frame might not be sent.
terminateOnDestroy = false;
duplex.destroy(err);
});

Expand Down Expand Up @@ -108,7 +119,8 @@ function createWebSocketStream(ws, options) {
if (!called) callback(err);
process.nextTick(emitClose, duplex);
});
ws.terminate();

if (terminateOnDestroy) ws.terminate();
};

duplex._final = function (callback) {
Expand Down
9 changes: 8 additions & 1 deletion test/create-websocket-stream.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@ describe('createWebSocketStream', () => {
});

it('reemits errors', (done) => {
let duplexCloseEventEmitted = false;
const wss = new WebSocket.Server({ port: 0 }, () => {
const ws = new WebSocket(`ws://localhost:${wss.address().port}`);
const duplex = createWebSocketStream(ws);
Expand All @@ -215,13 +216,19 @@ describe('createWebSocketStream', () => {
);

duplex.on('close', () => {
wss.close(done);
duplexCloseEventEmitted = true;
});
});
});

wss.on('connection', (ws) => {
ws._socket.write(Buffer.from([0x85, 0x00]));
ws.on('close', (code, reason) => {
assert.ok(duplexCloseEventEmitted);
assert.strictEqual(code, 1002);
assert.strictEqual(reason, '');
wss.close(done);
});
});
});

Expand Down

0 comments on commit 074e6a8

Please sign in to comment.