WIP | Try to fix Cancelation Task exception #1253
Conversation
|
I haven't investigated but it's worth a try. |
| @@ -2643,7 +2643,12 @@ protected override Task<DbDataReader> ExecuteDbDataReaderAsync(CommandBehavior b | |||
| /// <include file='../../../../../../../doc/snippets/Microsoft.Data.SqlClient/SqlCommand.xml' path='docs/members[@name="SqlCommand"]/ExecuteReaderAsync[@name="CancellationToken"]/*'/> | |||
| new public Task<SqlDataReader> ExecuteReaderAsync(CancellationToken cancellationToken) | |||
| { | |||
| return ExecuteReaderAsync(CommandBehavior.Default, cancellationToken); | |||
| Task<SqlDataReader> resultTask = ExecuteReaderAsync(CommandBehavior.Default, cancellationToken); | |||
| if (resultTask.IsCanceled) | |||
There was a problem hiding this comment.
The right solution here would be to go to where the TaskCanceledException is originally thrown (i.e. somewhere inside ExecuteReaderAsync), and make sure the correct exception with the correct token is thrown there.
(By the way, I don't think this code would work because of #26 - SqlClient currently doesn't throw TaskCanceledException but rather SqlException, so resultTask here would be faulted rather than canceled. But even if it did work, this is better fixed at the source)
There was a problem hiding this comment.
SqlClient currently doesn't throw TaskCanceledException but rather SqlException, so resultTask here would be faulted rather than canceled
A TaskCanceledException (wrapped in an AggregateException) is thrown and the task is canceled if the token is canceled before execution starts (i.e. at the beginning of ExecuteReaderAsync). If the token is canceled during execution, a similarly wrapped SqlException is thrown and the task is faulted. A lot of effort would be required to fix this properly. I already put a few weeks into it but was eventually blocked.
There was a problem hiding this comment.
Am I right that we'd need to somehow include the cancellation token in the SNIError so that it could be included when we throw?
Possible fix for #1250
If approved, same fixes should go to
SqlConnection. I have added to SqlConnection so far, but waiting to see others opinion to add it toInternalOpenWithRetryLogic.@roji and @Wraith2 could this be a possible fix for this issue?