Skip to content

Abort the request if close is called while connecting #956

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lpinca merged 1 commit into from
Jan 10, 2017
Merged

Abort the request if close is called while connecting #956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 28 additions & 38 deletions lib/WebSocket.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,10 +262,10 @@ class WebSocket extends EventEmitter {
*/
close (code, data) {
if (this.readyState === WebSocket.CLOSED) return;

if (this.readyState === WebSocket.CONNECTING) {
this.readyState = WebSocket.CLOSED;
return;
this._req.abort();
this.emit('error', new Error('closed before the connection is established'));
return this.finalize(true);
}

if (this.readyState === WebSocket.CLOSING) {
Expand Down Expand Up @@ -374,6 +374,11 @@ class WebSocket extends EventEmitter {
*/
terminate () {
if (this.readyState === WebSocket.CLOSED) return;
if (this.readyState === WebSocket.CONNECTING) {
this._req.abort();
this.emit('error', new Error('closed before the connection is established'));
return this.finalize(true);
}

if (this._socket) {
this.readyState = WebSocket.CLOSING;
Expand All @@ -385,8 +390,6 @@ class WebSocket extends EventEmitter {
//
clearTimeout(this._closeTimer);
this._closeTimer = setTimeout(this._finalize, closeTimeout, true);
} else if (this.readyState === WebSocket.CONNECTING) {
this.finalize(true);
}
}
}
Expand Down Expand Up @@ -610,43 +613,32 @@ function initAsClient (address, protocols, options) {

if (agent) requestOptions.agent = agent;

const req = httpObj.get(requestOptions);
this._req = httpObj.get(requestOptions);

this._req.on('error', (error) => {
if (this._req.aborted) return;
Copy link
Copy Markdown
Member Author

@lpinca lpinca Jan 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to avoid "socket hang up" errors which are emitted when a request is aborted and a response is not received.

We can remove the check as the error would be swallowed anyway but I think it's better to be explicit here.


req.on('error', (error) => {
this.emit('error', error);
this.finalize(error);
this.finalize(true);
});

req.on('response', (res) => {
var error;

if (!this.emit('unexpected-response', req, res)) {
error = new Error(`unexpected server response (${res.statusCode})`);
req.abort();
this.emit('error', error);
this._req.on('response', (res) => {
if (!this.emit('unexpected-response', this._req, res)) {
this._req.abort();
this.emit('error', new Error(`unexpected server response (${res.statusCode})`));
this.finalize(true);
}

this.finalize(error);
});

req.on('upgrade', (res, socket, head) => {
if (this.readyState === WebSocket.CLOSED) {
// client closed before server accepted connection
this.emit('close');
this.removeAllListeners();
socket.end();
return;
}

this._req.on('upgrade', (res, socket, head) => {
const digest = crypto.createHash('sha1')
.update(key + GUID, 'binary')
.digest('base64');

if (res.headers['sec-websocket-accept'] !== digest) {
socket.destroy();
this.emit('error', new Error('invalid server key'));
this.removeAllListeners();
socket.end();
return;
return this.finalize(true);
}

const serverProt = res.headers['sec-websocket-protocol'];
Expand All @@ -662,30 +654,28 @@ function initAsClient (address, protocols, options) {
}

if (protError) {
socket.destroy();
this.emit('error', new Error(protError));
this.removeAllListeners();
socket.end();
return;
return this.finalize(true);
}

if (serverProt) this.protocol = serverProt;

const serverExtensions = Extensions.parse(res.headers['sec-websocket-extensions']);

if (perMessageDeflate && serverExtensions[PerMessageDeflate.extensionName]) {
try {
perMessageDeflate.accept(serverExtensions[PerMessageDeflate.extensionName]);
} catch (err) {
socket.destroy();
this.emit('error', new Error('invalid extension parameter'));
this.removeAllListeners();
socket.end();
return;
return this.finalize(true);
}

this.extensions[PerMessageDeflate.extensionName] = perMessageDeflate;
}

this._req = null;
this.setSocket(socket, head);

req.removeAllListeners();
agent = null;
});
}
122 changes: 76 additions & 46 deletions test/WebSocket.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,39 +95,28 @@ describe('WebSocket', function () {
wss.on('connection', (ws) => ws.send('foobar'));
});

it('#url exposes the server url', function (done) {
server.createServer(++port, (srv) => {
const url = `ws://localhost:${port}`;
const ws = new WebSocket(url);

assert.strictEqual(ws.url, url);
it('#url exposes the server url', function () {
const url = `ws://localhost:${port}`;
const ws = new WebSocket(url, { agent: new CustomAgent() });

ws.on('close', () => srv.close(done));
ws.close();
});
assert.strictEqual(ws.url, url);
});

it('#protocolVersion exposes the protocol version', function (done) {
server.createServer(++port, (srv) => {
const ws = new WebSocket(`ws://localhost:${port}`);

assert.strictEqual(ws.protocolVersion, 13);

ws.on('close', () => srv.close(done));
ws.close();
it('#protocolVersion exposes the protocol version', function () {
const ws = new WebSocket(`ws://localhost:${port}`, {
agent: new CustomAgent()
});

assert.strictEqual(ws.protocolVersion, 13);
});

describe('#bufferedAmount', function () {
it('defaults to zero', function (done) {
server.createServer(++port, (srv) => {
const ws = new WebSocket(`ws://localhost:${port}`);

assert.strictEqual(ws.bufferedAmount, 0);

ws.on('close', () => srv.close(done));
ws.close();
it('defaults to zero', function () {
const ws = new WebSocket(`ws://localhost:${port}`, {
agent: new CustomAgent()
});

assert.strictEqual(ws.bufferedAmount, 0);
});

it('defaults to zero upon "open"', function (done) {
Expand Down Expand Up @@ -203,15 +192,12 @@ describe('WebSocket', function () {
});

describe('#readyState', function () {
it('defaults to connecting', function (done) {
server.createServer(++port, (srv) => {
const ws = new WebSocket(`ws://localhost:${port}`);

assert.strictEqual(ws.readyState, WebSocket.CONNECTING);

ws.on('close', () => srv.close(done));
ws.close();
it('defaults to connecting', function () {
const ws = new WebSocket(`ws://localhost:${port}`, {
agent: new CustomAgent()
});

assert.strictEqual(ws.readyState, WebSocket.CONNECTING);
});

it('set to open once connection is established', function (done) {
Expand All @@ -236,7 +222,7 @@ describe('WebSocket', function () {
srv.close(done);
});

ws.close(1001);
ws.on('open', () => ws.close(1001));
});
});

Expand All @@ -249,7 +235,7 @@ describe('WebSocket', function () {
srv.close(done);
});

ws.terminate();
ws.on('open', () => ws.terminate());
});
});
});
Expand Down Expand Up @@ -308,26 +294,68 @@ describe('WebSocket', function () {
});

describe('connection establishing', function () {
it('can disconnect before connection is established', function (done) {
server.createServer(++port, (srv) => {
it('can terminate before connection is established (1/2)', function (done) {
const wss = new WebSocketServer({ port: ++port }, () => {
const ws = new WebSocket(`ws://localhost:${port}`);

ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here'));
ws.on('close', () => srv.close(done));
ws.on('error', (err) => {
assert.ok(err instanceof Error);
assert.strictEqual(err.message, 'closed before the connection is established');
ws.on('close', () => wss.close(done));
});
ws.terminate();
});
});

it('can close before connection is established', function (done) {
server.createServer(++port, (srv) => {
it('can terminate before connection is established (2/2)', function (done) {
const wss = new WebSocketServer({
verifyClient: (info, cb) => setTimeout(cb, 300, true),
port: ++port
}, () => {
const ws = new WebSocket(`ws://localhost:${port}`);

ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here'));
ws.on('close', () => srv.close(done));
ws.on('error', (err) => {
assert.ok(err instanceof Error);
assert.strictEqual(err.message, 'closed before the connection is established');
ws.on('close', () => wss.close(done));
});
setTimeout(() => ws.terminate(), 150);
});
});

it('can close before connection is established (1/2)', function (done) {
const wss = new WebSocketServer({ port: ++port }, () => {
const ws = new WebSocket(`ws://localhost:${port}`);

ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here'));
ws.on('error', (err) => {
assert.ok(err instanceof Error);
assert.strictEqual(err.message, 'closed before the connection is established');
ws.on('close', () => wss.close(done));
});
ws.close(1001);
});
});

it('can close before connection is established (2/2)', function (done) {
const wss = new WebSocketServer({
verifyClient: (info, cb) => setTimeout(cb, 300, true),
port: ++port
}, () => {
const ws = new WebSocket(`ws://localhost:${port}`);

ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here'));
ws.on('error', (err) => {
assert.ok(err instanceof Error);
assert.strictEqual(err.message, 'closed before the connection is established');
ws.on('close', () => wss.close(done));
});
setTimeout(() => ws.close(1001), 150);
});
});

it('can handle error before request is upgraded', function (done) {
// Here, we don't create a server, to guarantee that the connection will
// fail before the request is upgraded
Expand Down Expand Up @@ -791,15 +819,17 @@ describe('WebSocket', function () {
});

it('before connect should pass error through callback, if present', function (done) {
server.createServer(++port, (srv) => {
const wss = new WebSocketServer({ port: ++port }, () => {
const ws = new WebSocket(`ws://localhost:${port}`);

ws.send('hi', (error) => {
assert.ok(error instanceof Error);
srv.close(done);
ws.terminate();
ws.send('hi', (err) => {
assert.ok(err instanceof Error);
assert.strictEqual(err.message, 'not opened');
ws.on('close', () => wss.close(done));
});
});

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

it('without data should be successful', function (done) {
Expand Down
38 changes: 18 additions & 20 deletions test/WebSocketServer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,38 +202,37 @@ describe('WebSocketServer', function () {
const ws = new WebSocket(`ws://localhost:${port}`);
});

wss.on('connection', (client) => {
wss.on('connection', (ws) => {
assert.strictEqual(wss.clients.size, 1);
wss.close();
done();
wss.close(done);
});
});

it('can be disabled', function (done) {
const wss = new WebSocketServer({ port: ++port, clientTracking: false }, () => {
assert.strictEqual(wss.clients, undefined);
const ws = new WebSocket(`ws://localhost:${port}`);

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

wss.on('connection', (client) => {
wss.on('connection', (ws) => {
assert.strictEqual(wss.clients, undefined);
wss.close();
done();
ws.on('close', () => wss.close(done));
});
});

it('is updated when client terminates the connection', function (done) {
const wss = new WebSocketServer({ port: ++port }, () => {
const ws = new WebSocket(`ws://localhost:${port}`);

wss.on('connection', (client) => {
client.on('close', () => {
assert.strictEqual(wss.clients.size, 0);
wss.close();
done();
});
ws.on('open', () => ws.terminate());
});

ws.close();
wss.on('connection', (ws) => {
ws.on('close', () => {
assert.strictEqual(wss.clients.size, 0);
wss.close(done);
});
});
});
Expand All @@ -242,14 +241,13 @@ describe('WebSocketServer', function () {
const wss = new WebSocketServer({ port: ++port }, () => {
const ws = new WebSocket(`ws://localhost:${port}`);

wss.on('connection', (client) => {
client.on('close', () => {
assert.strictEqual(wss.clients.size, 0);
wss.close();
done();
});
ws.on('open', () => ws.close());
});

ws.close();
wss.on('connection', (ws) => {
ws.on('close', () => {
assert.strictEqual(wss.clients.size, 0);
wss.close(done);
});
});
});
Expand Down
⚡