Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix | Fixes wrong data errors due to exceptions not handled in time #894

Closed
wants to merge 3 commits into from
Closed

Fix | Fixes wrong data errors due to exceptions not handled in time #894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
067c280
Final fix: Check and throw exception before passing resultset
cheenamalhotra Feb 4, 2021
ce17f83
Fix NetFx too
cheenamalhotra Feb 4, 2021
50565d1
Fix Non Query too
cheenamalhotra Feb 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1368,6 +1368,7 @@ private int EndExecuteNonQueryInternal(IAsyncResult asyncResult)

try
{
CheckThrowSNIException();
statistics = SqlStatistics.StartTimer(Statistics);
int result = (int)InternalEndExecuteNonQuery(asyncResult, isInternal: false, endMethod: nameof(EndExecuteNonQuery));
success = true;
Expand Down Expand Up @@ -1779,6 +1780,7 @@ private XmlReader EndExecuteXmlReaderInternal(IAsyncResult asyncResult)
int? sqlExceptionNumber = null;
try
{
CheckThrowSNIException();
success = true;
return CompleteXmlReader(InternalEndExecuteReader(asyncResult, false, nameof(EndExecuteXmlReader)), true);
}
Expand Down Expand Up @@ -1973,6 +1975,7 @@ private SqlDataReader EndExecuteReaderInternal(IAsyncResult asyncResult)
int? sqlExceptionNumber = null;
try
{
CheckThrowSNIException();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love for this to be the final fix to this issue. But how come adding a call to CheckThrowSNIException() at these locations makes a difference when the same method is already being called not much later in the flow?

From here, -> InternalEndExecuteReader (about 4 statements into it) -> CheckThrowSNIException()

I wonder if VerifyEndExecuteState is stopping flow before it gets to CheckThrowSNIException.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true. But this flow actually completes and reader is provided to user.
I'm looking more into why CheckThrow is swallowing that exception and not throwing it in that particular case.

Manual stepping does not reproduce that error, and not all cases that timeout are bypassed as we have good exception cases too.. I'll continue scanning and tracking again. It's far more dense that it seems 🙈

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In InternalEndExecuteReader before it checks for errors it calls WaitForAsyncResults which resets the _networkPacketSource which is where the OnTimeout function puts the exception that we want to find and throw. It certainly changes the behaviour in one of the downstream callee's of the poorly named CheckThrowSNIException (it doesn't throw...).

If the original author of all this isn't in an asylum at the moment I expect they're currently located in a hidden base at the base of a volcano planning world domination.

success = true;
statistics = SqlStatistics.StartTimer(Statistics);
return InternalEndExecuteReader(asyncResult, false, nameof(EndExecuteReader));
Expand Down
3 changes: 3 additions & 0 deletions src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1720,6 +1720,7 @@ private int EndExecuteNonQueryInternal(IAsyncResult asyncResult)
int? sqlExceptionNumber = null;
try
{
CheckThrowSNIException();
statistics = SqlStatistics.StartTimer(Statistics);
int result = (int)InternalEndExecuteNonQuery(asyncResult, ADP.EndExecuteNonQuery, isInternal: false);
success = true;
Expand Down Expand Up @@ -2230,6 +2231,7 @@ private XmlReader EndExecuteXmlReaderInternal(IAsyncResult asyncResult)
int? sqlExceptionNumber = null;
try
{
CheckThrowSNIException();
XmlReader result = CompleteXmlReader(InternalEndExecuteReader(asyncResult, ADP.EndExecuteXmlReader, isInternal: false), true);
success = true;
return result;
Expand Down Expand Up @@ -2486,6 +2488,7 @@ private SqlDataReader EndExecuteReaderInternal(IAsyncResult asyncResult)
int? sqlExceptionNumber = null;
try
{
CheckThrowSNIException();
statistics = SqlStatistics.StartTimer(Statistics);
SqlDataReader result = InternalEndExecuteReader(asyncResult, ADP.EndExecuteReader, isInternal: false);
success = true;
Expand Down