Calling cancel in SENT_ATTENTION returns false and writes debug error

[tvrprasad]

This should not be an error and we should return true. User has no way to know that the connection is in SENT_ATTENTION state and even otherwise it should not be error.

[ElfenLiedGH]

@tvrprasad i saw it when "requestTimeout" was end.

[tvrprasad]

@ElfenLiedGH I don't quite follow. Can you please elaborate? Thanks.

[chdh]

@tvrprasad i saw it when "requestTimeout" was end.

@ElfenLiedGH I don't quite follow. Can you please elaborate? Thanks

It's exactly the program logic sequence that I described in a comment for #518:

• The application calls connection.execSql() to start a query.
• The query takes longer than expected. A request timeout occurs. The driver sends ATTENTION to the server and enters the SENT_ATTENTION state. The application has no way of knowing this.
• While the driver is in the SENT_ATTENTION state and waiting for the attention response from the server, the application calls Connection.cancel(). The current implementation of the cancel() method considers this an error, writes the debug message "Requests can only be canceled in the SentClientRequest state, not the SentAttention state" and returns false.

I still think that cancel() and similar public API methods should just ignore such conditions anyway and keep quiet. Moreover, the API doc does not mention anything about the return value (true or false) from cancel().

[tvrprasad]

@chdh I agreed with you on that specific comment, which is why I opened this issue :-)

[Hadis-Knj]

If application calls connection.cancel() in row event where all rows have been received before server gets ATTENTION message, it outputs all columns and throws RequestError: Canceled. error, I think it should not throw an error and just ignore.

[chdh]

I think it should not throw an error and just ignore.

I disagree. Tedious does not really throw an error, it just passes a RequestError object to the callback function. This is in accordance with the TDS protocol specification 3.2.5.8 Sent Attention State:
"... indicate the completion of the query to the upper layer together with the cause of the Attention ...".

[Hadis-Knj]

in this scenario, after executing the request, state changes to SentClientRequest, and application receives all packets. row event is emitted, and then application makes a call to connection.cancel() in row event handler. so state changes to SentClientRequest -> SentAttention, we output all the results in the row, and then receives ack from server and state changes SentAttention -> LoggedIn , and then it errors out. since we have received the ack from server, client should not error out.
tested the same query in .Net and JDBC and didn't get an error

[chdh]

@Hadis-Fard I don't understand. Could you provide a test case?

[Hadis-Knj]

@chdh in https://github.com/tediousjs/tedious/blob/master/src/connection.js#L1667 doesn't make sense to error out if the ack is received and is a valid cancel scenario, why should it throw debug error if the ack is received and request is canceled? what is the case that calling cancel doesn't throw an error?

[Suraiya-Hameed]

If the response is valid,

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 in 3.2.4, the upper-layer mentioned is MARS, so its currently not applicable for tedious, need to check on Cancel Timer though.

Error should be thrown only if response received is invalid.

If the response received from the server is not structurally valid, then the TDS client MUST close the underlying transport connection, indicate an error to the upper layer, and enter the "Final State" state.

[chdh]

@v-suhame In the case of Tedious, the "upper layer" is the application program. 3.2.4 is about events triggered by (i.e. received from) the upper layer. In our case this is the Connection.cancel() call. The application (upper layer) has called Connection.cancel(). In the language of the TDS specification, this means that the upper layer has sent a Cancel Request.

3.2.4 is relevant for Tedious, even if MARS is not supported.

The cause of the Attention is either that the application has called Connection.cancel() or that a timeout occurred (requestTimeout() in connection.js). In both cases, the cause has to reported to the upper layer (according to 3.2.5.8), which is the application in our case. This is done by passing a RequestError object to the application-provided callback function (line 1668 in connection.js).

Tedious does not throw an error, it only creates a RequestError object and passes it as a parameter to the user-provided callback function. This is what the specification calls "indicate the completion of the query together with the cause of the Attention". Tedious uses a RequestError object to encode the "cause of the Attention" and passes this information to the application via the callback function.

(And in our case, the response is always "structurally valid". Otherwise the database connection is closed.)

[arthurschreiber]

This will be fixed via #846. 😬

[arthurschreiber]

🎉 This issue has been resolved in version 5.0.0 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀