Skip to content
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.

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

allow client to specify handshake request timeout #1177

Merged
merged 5 commits into from Jul 27, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/ws.md
Expand Up @@ -170,6 +170,7 @@ This class represents a WebSocket. It extends the `EventEmitter`.
- `protocols` {String|Array} The list of subprotocols.
- `options` {Object}
- `protocol` {String} Value of the `Sec-WebSocket-Protocol` header.
- `handshakeTimeout` {Number} Timeout in milliseconds for the handshake request.
- `perMessageDeflate` {Boolean|Object} Enable/disable permessage-deflate.
- `localAddress` {String} Local interface to bind for network connections.
- `protocolVersion` {Number} Value of the `Sec-WebSocket-Version` header.
Expand Down
10 changes: 10 additions & 0 deletions lib/WebSocket.js
Expand Up @@ -471,6 +471,7 @@ function initAsServerClient (socket, head, options) {
* @param {Object} options Connection options
* @param {String} options.protocol Value of the `Sec-WebSocket-Protocol` header
* @param {(Boolean|Object)} options.perMessageDeflate Enable/disable permessage-deflate
* @param {Number} options.handshakeTimeout Timeout in milliseconds for the handshake request
* @param {String} options.localAddress Local interface to bind for network connections
* @param {Number} options.protocolVersion Value of the `Sec-WebSocket-Version` header
* @param {Object} options.headers An object containing request headers
Expand All @@ -493,6 +494,7 @@ function initAsClient (address, protocols, options) {
protocolVersion: protocolVersions[1],
protocol: protocols.join(','),
perMessageDeflate: true,
handshakeTimeout: null,
localAddress: null,
headers: null,
family: null,
Expand Down Expand Up @@ -632,6 +634,14 @@ function initAsClient (address, protocols, options) {

this._req = httpObj.get(requestOptions);

if (options.handshakeTimeout) {
this._req.setTimeout(options.handshakeTimeout, () => {
Copy link
Member

@lpinca lpinca Jul 25, 2017

Choose a reason for hiding this comment

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

This may not work exactly as you expect but it's fine in most cases. All request.setTimeout() does is calling socket.setTimeout() on the request socket.

This means that the timeout expires only when there are x milliseconds of inactivity. It's possible that the response arrives after the specified timeout with no 'timeout' event emitted.

this._req.abort();
this.emit('error', new Error('timeout'));
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a better error message? Like I don't know "opening handshake has timed out".

this.finalize(true);
});
}

this._req.on('error', (error) => {
if (this._req.aborted) return;

Expand Down
24 changes: 24 additions & 0 deletions test/WebSocket.test.js
Expand Up @@ -461,6 +461,30 @@ describe('WebSocket', function () {
req.abort();
});
});

it('error is emitted if server response exceeds timeout', function (done) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd use "emits an error if the opening handshake timeout expires".

const timeout = 100;
server.once('upgrade', (req, socket) => {
Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to just make the listener a noop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that, but for some reason it doesn't work.

Copy link
Member

Choose a reason for hiding this comment

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

Does it fail on the afterEach() hook?

Copy link
Contributor Author

@id0Sch id0Sch Jul 25, 2017

Choose a reason for hiding this comment

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

yes

Copy link
Member

@lpinca lpinca Jul 25, 2017

Choose a reason for hiding this comment

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

Ok, this is becuase allowHalfOpen is set to true and the server socket is not automatically closed.

This should fix the issue:

server.once('upgrade', (req, socket) => socket.on('end', socket.end));

setTimeout(() => {
socket.end(
`HTTP/1.1 401 ${http.STATUS_CODES[401]}\r\n` +
'Connection: close\r\n' +
'Content-type: text/html\r\n' +
`Content-Length: ${http.STATUS_CODES[401].length}\r\n` +
'\r\n'
);
}, timeout * 1.5);
});

const ws = new WebSocket(`ws://localhost:${port}`, null, { handshakeTimeout: timeout });
Copy link
Member

Choose a reason for hiding this comment

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

Can you please reduce the length of this line?

const ws = new WebSocket(`ws://localhost:${port}`, null, {
  handshakeTimeout: timeout
});


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, 'timeout');
done();
});
});
});

describe('connection with query string', function () {
Expand Down