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

Conversation

id0Sch
Copy link
Contributor

@id0Sch id0Sch commented Jul 25, 2017

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!

lib/WebSocket.js Outdated
if (options.handshakeTimeout) {
this._req.setTimeout(options.handshakeTimeout, () => {
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".

@@ -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".


it('error is emitted if server response exceeds timeout', function (done) {
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));

}, 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
});

@@ -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.

- changes name of test
- changes the error message that is returned and also adds an error code
@id0Sch
Copy link
Contributor Author

id0Sch commented Jul 25, 2017

@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';
Copy link
Member

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.

Copy link
Contributor Author

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.

Ido schachter added 2 commits July 25, 2017 19:07
removed code
proper close of server socket
server.once('upgrade', (req, socket) => socket.on('end', socket.end));

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.

One last nit: would you mind to inline the value and remove the variable?

Copy link
Contributor Author

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
@lpinca
Copy link
Member

lpinca commented Jul 26, 2017

@websockets/admin LGTY?

@lpinca lpinca merged commit b320169 into websockets:master Jul 27, 2017
@lpinca
Copy link
Member

lpinca commented Jul 27, 2017

Thank you.

@id0Sch
Copy link
Contributor Author

id0Sch commented Jul 27, 2017

@lpinca thanks for being so helpful and fast moving!
i'ts been a pleasure contributing to this awesome project!
keep up the good work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants