WIP: Improve Async Paths #335
Conversation
| // Account for split packets | ||
| while (readBytes < TdsEnums.HEADER_LEN) | ||
| { | ||
| readBytes += async ? |
There was a problem hiding this comment.
This can infinite loop if the connection is closed and the read returns 0 and doesn't throw, Are closed connections guaranteed to throw or return non-zero?
There was a problem hiding this comment.
Hmm... the existing loop also has this issue.
Will fix in both. Good spot
There was a problem hiding this comment.
Someone else mentioned it elsewhere before I just haven't got around to looking at it in any depth yet so since you're here anyway I thought it worth mentioning.
There may not be a clear way to "fix" it from what I remember. I'm not sure what should happen if you end up in a partial packet read because it's below ssl stream which is going to try and decode anything you return in the buffer which can error that looks like a transport error when in fact it's connection closed. See what you think.
|
Looks good apart from one query on preexisting code pattern. |
|
I believe work is in progress, if it's taking longer to fix the issue we might wanna consider reverting the previous PR to resolve deadlocks for end users, specially in System.Data.SqlClient as they might be preparing for next release in January, also here in Microsoft.Data.SqlClient for users impacted and our next planned preview release is in January. And with this PR (once we get the fix working) we can introduce the previous changes again. |
|
yes, its probably best to just reverse the changes if that resolves the issue |
Fixes dotnet#262 by reverting dotnet/corefx#34184 until PR dotnet#335 is ready with complete fix.
Fixes #262 by reverting dotnet/corefx#34184 until PR #335 is ready with complete fix.
| catch (Exception ex) | ||
| { | ||
| t = Task.FromException<int>(ex); | ||
| } | ||
|
|
||
| if ((t.Status & TaskStatus.RanToCompletion) != 0) |
There was a problem hiding this comment.
The old code (t.Status & TaskStatus.RanToCompletion) != 0 in SNIPacket.NetStandard.cs looks strange, since TaskStatus is not a flag, so this will even be true for TaskStatus.WaitingForActivation, maybe it should be t.Status == TaskStatus.RanToCompletion instead to follow the logic in netcoreapp?
There was a problem hiding this comment.
Nice catch. Looking at the enum values that'll let fault and cancel through. I'm surprised visual studio doesn't complain about doing bitwise operations on a non-[Flag] enum.
| catch (Exception ex) | ||
| { | ||
| t = Task.FromException(ex); | ||
| } | ||
|
|
||
| if ((t.Status & TaskStatus.RanToCompletion) != 0) |
|
Closing as it seems this PR is not mergeable due to some deep changes on the main branch. Feel free to open a new PR. |
Override other Stream Async methods
Catch exceptions and wrap in Task incase thrown synchronously
contributes to #262