Stop request timer when first packet of response message is received #530
Conversation
| @@ -567,7 +570,8 @@ class Connection extends EventEmitter { | |||
|
|
|||
| clearRequestTimer() { | |||
| if (this.requestTimer) { | |||
| return clearTimeout(this.requestTimer); | |||
| clearTimeout(this.requestTimer); | |||
| this.requestTimer = undefined; | |||
There was a problem hiding this comment.
For consistency with the rest of the file, please change this to:
this.requestTimer = void 0;There was a problem hiding this comment.
Remove the call to clearTimeout under SENT_CLIENT_REQUEST:message. We don't need that now.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
'undefined' is preferred over 'void 0'. 'void 0' is a relic of the CoffeeScript to JavaScript conversion. 😔
There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Probably not worth it, considering the real pause/resume is coming soon and we'll have clean tests.
src/connection.js
Outdated
| @@ -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(); | |||
There was a problem hiding this comment.
This technically belongs under SENT_CLIENT_REQUEST.data. Can't come up with a case where this breaks anything though...
| return this.sendDataToTokenStreamParser(data); | ||
| }, | ||
| message: function() { | ||
| this.clearRequestTimer(); |
There was a problem hiding this comment.
clearRequestTimer() is now called in the exit function of the SENT_CLIENT_REQUEST state.
| @@ -1001,7 +1005,6 @@ class Connection extends EventEmitter { | |||
| this.debug.log(message); | |||
| return false; | |||
| } else { | |||
| this.clearRequestTimer(); | |||
There was a problem hiding this comment.
clearRequestTimer() is now called in the exit function of the SENT_CLIENT_REQUEST state.
This behavior is according to the TDS standard.