Skip to content

Commit

Permalink
[fix] Make WebSocket#close() set the close timer immediately
Browse files Browse the repository at this point in the history
If there is buffered data to write and the stream is not flowing, it
is possible that the callback of `Sender.prototype.close()` is never
called.
  • Loading branch information
lpinca committed Mar 26, 2019
1 parent 297f56d commit aa1dcd5
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 37 deletions.
29 changes: 13 additions & 16 deletions lib/websocket.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ const {

const readyStates = ['CONNECTING', 'OPEN', 'CLOSING', 'CLOSED'];
const protocolVersions = [8, 13];
const closeTimeout = 30 * 1000; // Allow 30 seconds to terminate the connection cleanly.
const closeTimeout = 30 * 1000;

/**
* Class representing a WebSocket.
Expand Down Expand Up @@ -87,8 +87,9 @@ class WebSocket extends EventEmitter {
}

/**
* This deviates from the WHATWG interface since ws doesn't support the required
* default "blob" type (instead we define a custom "nodebuffer" type).
* This deviates from the WHATWG interface since ws doesn't support the
* required default "blob" type (instead we define a custom "nodebuffer"
* type).
*
* @type {String}
*/
Expand Down Expand Up @@ -230,20 +231,16 @@ class WebSocket extends EventEmitter {
if (err) return;

this._closeFrameSent = true;

if (this._socket.writable) {
if (this._closeFrameReceived) this._socket.end();

//
// Ensure that the connection is closed even if the closing handshake
// fails.
//
this._closeTimer = setTimeout(
this._socket.destroy.bind(this._socket),
closeTimeout
);
}
if (this._closeFrameReceived) this._socket.end();
});

//
// Specify a timeout for the closing handshake to complete.
//
this._closeTimer = setTimeout(
this._socket.destroy.bind(this._socket),
closeTimeout
);
}

/**
Expand Down
52 changes: 31 additions & 21 deletions test/websocket.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1305,27 +1305,6 @@ describe('WebSocket', () => {
});
});

it('ends connection to the server', (done) => {
const wss = new WebSocket.Server(
{
clientTracking: false,
port: 0
},
() => {
const ws = new WebSocket(`ws://localhost:${wss.address().port}`);

ws.on('open', () => {
ws.on('close', (code, reason) => {
assert.strictEqual(reason, 'some reason');
assert.strictEqual(code, 1000);
wss.close(done);
});
ws.close(1000, 'some reason');
});
}
);
});

it('permits all buffered data to be delivered', (done) => {
const wss = new WebSocket.Server(
{
Expand Down Expand Up @@ -1383,6 +1362,37 @@ describe('WebSocket', () => {

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

it('sets a timer for the closing handshake to complete', (done) => {
const wss = new WebSocket.Server({ port: 0 }, () => {
const ws = new WebSocket(`ws://localhost:${wss.address().port}`);

ws.on('close', (code, reason) => {
assert.strictEqual(code, 1000);
assert.strictEqual(reason, 'some reason');
wss.close(done);
});

ws.on('open', () => {
let callbackCalled = false;

assert.strictEqual(ws._closeTimer, null);

ws.send('foo', () => {
callbackCalled = true;
});

ws.close(1000, 'some reason');

//
// Check that the close timer is set even if the `Sender.close()`
// callback is not called.
//
assert.strictEqual(callbackCalled, false);
assert.strictEqual(ws._closeTimer._idleTimeout, 30000);
});
});
});
});

describe('#terminate', () => {
Expand Down

0 comments on commit aa1dcd5

Please sign in to comment.