-
Notifications
You must be signed in to change notification settings - Fork 290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Improve Async Paths #335
Conversation
// Account for split packets | ||
while (readBytes < TdsEnums.HEADER_LEN) | ||
{ | ||
readBytes += async ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... the existing loop also has this issue.
Will fix in both. Good spot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And also for this one
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