Skip to content

Implement Request Timeouts #146

Merged
merged 2 commits into from Jun 15, 2014

2 participants

@arthurschreiber
Collaborator

This implements the missing request timeout functionality, by reusing the request cancellation code.

Unfortunately, this code comes without tests, because writing tests using nodeunit is a real pain in the butt and I was not yet able to allocate any time to move the test suite to mocha.

The functionality itself should work fine, as we've been running it for quite a while on our production systems.

The implementation was kindly provided by Jeevan Kumar Jagadesh.

@arthurschreiber
Collaborator

@patriksimek @pekim Any comments?

@arthurschreiber arthurschreiber Implement Request Timeouts.
The implementation was kindly provided by Jeevan Kumar Jagadesh.
0b90f6b
@patriksimek
Collaborator

Thanks for PR Arthur, I need to test it and write some tests first. No matter how painful it is, it needs to be done. I will do it asap.

@arthurschreiber
Collaborator

No matter how painful it is, it needs to be done.

Haha, I fully understand that. It's just really hard to switch from mocha (which we use for our application) over to something crazy like nodeunit. I actually do have integration tests for this functionality, but they're part of our applications test suite, so extracting them is not a task of just a few minutes.

Mainly, I'd like to get a feedback if the general design is sane or not. As I said, we've been using this in production for some while and it seems to be doing what it's supposed to do, but I'm not a 100% sure on some of the implementation details.

I'll try to put some more specific comments in the diff.

@arthurschreiber arthurschreiber commented on an outdated diff Apr 17, 2014
src/connection.coffee
@@ -330,16 +333,18 @@ class Connection extends EventEmitter
)
@tokenStreamParser.on('done', (token) =>
if @request
- @request.emit('done', token.rowCount, token.more, @request.rows)
-
- if token.rowCount != undefined
- @request.rowCount += token.rowCount
+ # 3.2.5.7 Sent Attention State
+ # Discard any data contained in the response
+ if @state == @STATE.SENT_ATTENTION
@arthurschreiber
Collaborator

I'm not sure whether this really discards all data coming in. E.g. row events are still fired, for as long as they are received. Should I also change the row (and maybe other handlers) to ignore any incoming data while in the SENT_ATTENTION state?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arthurschreiber arthurschreiber commented on the diff Apr 17, 2014
src/connection.coffee
@@ -126,6 +126,7 @@ class Connection extends EventEmitter
data: (data) ->
@sendDataToTokenStreamParser(data)
message: ->
+ @clearRequestTimer()
@arthurschreiber
Collaborator

Here, I was not too sure where to put the @clearRequestTimer() call. Is this the correct place? From what I understand, the message handler is executed when all response data was received (EOM). Does that make sense to reset the timer here, or should that happen in the data eventhandler? I didn't find any more specific information in the TDS documentation.

@patriksimek
Collaborator
patriksimek added a note Apr 17, 2014

This should be ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@patriksimek patriksimek and 1 other commented on an outdated diff Apr 17, 2014
src/connection.coffee
sqlRequest.callback(RequestError("Canceled.", 'ECANCEL'))
-
- # else: skip all messages, now we are only interested about cancel acknowledgement (2.2.1.6)
@patriksimek
Collaborator
patriksimek added a note Apr 17, 2014

I'm not sure with this part of code. As documentation says:

The client can interrupt and cancel the current request by sending an Attention message. This is also known as out-of-band data, but any TDS packet that is currently being sent MUST be finished before sending the Attention message. After the client sends an Attention message, the client MUST read until it receives an Attention acknowledgment.

I believe that message method can be called multiple times here... once with currently sent TDS packet and once with Attention TDS packet. This is something I need to test to find out how does it work exactly.

@arthurschreiber
Collaborator

Please see 3.2.5.7 http://msdn.microsoft.com/en-us/library/dd340677.aspx:

If the response is structurally valid and it does not acknowledge the Attention as described in section 2.2.1.6, then the TDS client MUST discard any data contained in the response and remain in the "Sent Attention" state.

If the response is structurally valid and it acknowledges the Attention as described in section 2.2.1.6, then the TDS client MUST discard any data contained in the response, indicate the completion of the query to the upper layer together with the cause of the Attention (either an upper-layer cancellation as described in section 3.2.4 or query timeout as described in section 3.2.2), and enter the "Logged In" state.

Do we maybe need a flag on the request that signals that an attention acknowledgement was received for the current request before we switch back into the LOGGED_IN state?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@patriksimek patriksimek added the feature label Apr 17, 2014
@arthurschreiber arthurschreiber Request Timeouts: Fixup timeout handling.
This introduces a new "attention" event that is emitted if we receive
a done token with the attention flag set.

This cleans up the implementation a bit and actually fixes an issue
where we would switch back into the LoggedInState too early because we
did not actually wait for the attention acknowledgement.
09fd8a8
@arthurschreiber
Collaborator

Hey @patriksimek,

I just pushed another change that fixed some issue that existed in the initial implementation. Can you take another look?

@patriksimek patriksimek merged commit 879eb24 into pekim:master Jun 15, 2014

1 check passed

Details continuous-integration/travis-ci The Travis CI build passed
@patriksimek
Collaborator

Thanks @arthurschreiber, everything seems ok to me, I will write some tests for it.

@arthurschreiber
Collaborator

I do have some tests. But I've not put them in this PR because they're written using mocha. I hope to get the test rewrite with mocha done soon, and will then add the tests I have for the timeout functionality. 😄

@arthurschreiber arthurschreiber deleted the arthurschreiber:add-request-timeout branch Jun 15, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.