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
Conversation
lib/WebSocket.js
Outdated
if (options.handshakeTimeout) { | ||
this._req.setTimeout(options.handshakeTimeout, () => { | ||
this._req.abort(); | ||
this.emit('error', new Error('timeout')); |
There was a problem hiding this comment.
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".
test/WebSocket.test.js
Outdated
@@ -461,6 +461,30 @@ describe('WebSocket', function () { | |||
req.abort(); | |||
}); | |||
}); | |||
|
|||
it('error is emitted if server response exceeds timeout', function (done) { |
There was a problem hiding this comment.
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".
test/WebSocket.test.js
Outdated
|
||
it('error is emitted if server response exceeds timeout', function (done) { | ||
const timeout = 100; | ||
server.once('upgrade', (req, socket) => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
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));
test/WebSocket.test.js
Outdated
}, timeout * 1.5); | ||
}); | ||
|
||
const ws = new WebSocket(`ws://localhost:${port}`, null, { handshakeTimeout: timeout }); |
There was a problem hiding this comment.
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
});
@@ -632,6 +634,14 @@ function initAsClient (address, protocols, options) { | |||
|
|||
this._req = httpObj.get(requestOptions); | |||
|
|||
if (options.handshakeTimeout) { | |||
this._req.setTimeout(options.handshakeTimeout, () => { |
There was a problem hiding this comment.
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.
@lpinca thanks for the quick reply , I indeed suck at picking out message strings and such. |
lib/WebSocket.js
Outdated
if (options.handshakeTimeout) { | ||
this._req.setTimeout(options.handshakeTimeout, () => { | ||
const err = new Error('opening handshake has timed out'); | ||
err.code = 'TIMEOUT'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need code
? It's useful and many Node.js errors have this property but currently we don't have this property on our errors. I prefer to not add it for consistency and maybe revisit later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, thought since the message is a sentence adding code could be a better way to "locate" the error, but all good.
proper close of server socket
test/WebSocket.test.js
Outdated
server.once('upgrade', (req, socket) => socket.on('end', socket.end)); | ||
|
||
const ws = new WebSocket(`ws://localhost:${port}`, null, { | ||
handshakeTimeout: timeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last nit: would you mind to inline the value and remove the variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that it's not used, no problem.
removed single used variable
@websockets/admin LGTY? |
Thank you. |
@lpinca thanks for being so helpful and fast moving! |
Hi,
I have the need to control the connection timeout when initialising the connection,
I did my best to fall into your coding style and added the option in the documentation and added a unit test.
Hoping you will find this to your liking,
Let me know if there's anything I need to change, thanks!