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.

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

Stop request timer when first packet of response message is received #530

Merged
merged 2 commits into from
Mar 8, 2017
Merged

Stop request timer when first packet of response message is received #530

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
653344e
Stop request timer when first packet of response message is received
chdh Mar 7, 2017
82f2e38
Clean up request timer calls
chdh Mar 8, 2017
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
8 changes: 6 additions & 2 deletions src/connection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,10 @@ class Connection extends EventEmitter {
this.socket.on('close', this.socketClose);
this.socket.on('end', this.socketEnd);
this.messageIo = new MessageIO(this.socket, this.config.options.packetSize, this.debug);
this.messageIo.on('data', (data) => { this.dispatchEvent('data', data); });
this.messageIo.on('data', (data) => {
this.clearRequestTimer();
Copy link
Contributor

Choose a reason for hiding this comment

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

This technically belongs under SENT_CLIENT_REQUEST.data. Can't come up with a case where this breaks anything though...

this.dispatchEvent('data', data);
});
this.messageIo.on('message', () => {
return this.dispatchEvent('message');
});
Expand Down Expand Up @@ -567,7 +570,8 @@ class Connection extends EventEmitter {

clearRequestTimer() {
if (this.requestTimer) {
return clearTimeout(this.requestTimer);
clearTimeout(this.requestTimer);
this.requestTimer = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with the rest of the file, please change this to:

this.requestTimer = void 0;

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the call to clearTimeout under SENT_CLIENT_REQUEST:message. We don't need that now.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have a requestTimeout integration test in connection-test.coffee that tests the old behavior. It's still relevant, so we can keep it.

To test the new behavior, we can add a test that'll sleep for a little over the requestTimeout amount in the first invocation of 'row' event handler. With old behavior that would cause a timeout, whereas with the new behavior it should not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

'undefined' is preferred over 'void 0'. 'void 0' is a relic of the CoffeeScript to JavaScript conversion. 😔

Copy link
Contributor

Choose a reason for hiding this comment

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

This thread seems to suggest 'void 0' is safer - http://stackoverflow.com/questions/5716976/javascript-undefined-vs-void-0.

If we like 'undefined' I'll send a PR to change all 'void 0' s to undefined for consistency. Otherwise it'll keep bugging me :-)

Copy link
Collaborator Author

@chdh chdh Mar 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's not possible to design a reasonable and better test case without calling Request.pause(). But you could try yourself.

That would be too late, isn't it?

I meant to move the existing call of clearRequestTimer() from STATE.SENT_CLIENT_REQUEST.events.message to STATE.SENT_CLIENT_REQUEST.exit, in addition to the new call. That way we would have a fail-safe for future code modifications. Additionally it would also be safe to call clearRequestTimer() at the start of createRequestTimer(). But with the current situation, everything looks OK.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's not possible to design a reasonable and better test case without calling Request.pause(). But you could try yourself.

I tried and you're right. Sleep in 'row' does not help. 'message' event gets handled before timeout callback is invoked and clears the timer.

I meant to move the existing call of clearRequestTimer() from STATE.SENT_CLIENT_REQUEST.events.message to STATE.SENT_CLIENT_REQUEST.exit, in addition to the new call. That way we would have a fail-safe for future code modifications.

That makes sense. Please go ahead and make that update.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we like 'undefined' I'll send a PR to change all 'void 0' s to undefined for consistency. Otherwise it'll keep bugging me :-)

This would be great. There's quite a bit of leftovers from that conversion, and getting those cleaned up would be 💖.

void 0 was safer in ye olde days of ES3 without the 'use strict', where you could do var undefined = true and break all code that used undefined. With the 'use strict' directive, redefinition of undefined is no longer allowed and will throw an error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we could test a timeout by pausing one of the underlying streams after receiving the first row (and before receiving it). It's pretty ugly test case and I'm not sure whether testing this is worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not worth it, considering the real pause/resume is coming soon and we'll have clean tests.

}
}

Expand Down