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.

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

Convert ExecuteNonQueryAsync to use async context object #1692

Merged
merged 2 commits into from
Oct 5, 2022

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Aug 4, 2022

fixes #1682

This addresses the incorrect handling of cancellation token registrations in SqlCommand methods ExecuteNonQuery and ExecuteReader, I didn't change ExecuteScalar because it should be done in a separate PR and it's just ExecuteReader wearing a fake nose a glasses, people should use ExexuteReader

Background information: There are a number of async api calls on SqlDataReader which have been changed to avoid the use of language provided lambda closures and instead use explicit context objects. This process makes the code more complicated because it splits apart the code into multiple functions so that instance delegate allocations can be avoided. This pattern is no longer needed if we use language provided static lambdas and the code can be much cleaner and better contained. This PR converts the ExecuteNonQuery function to use this explicity context pattern and uses static lambdas to doi t.

This PR also changes all existing context objects to use an explicit type for the IDispoable so that we can avoid boxing the disposable. This change forces any existing context types which were declared like this: AAsyncContext<TOwner,TResult> to be split apart into two types,

  1. AAsyncContext<TOwner,TResult,TDisposable> which has a strong disposable type and is used at call sites that know what the disposable is, mostly setup locations.
  2. AAsyncBaseContext<TOwner,TResult> which is used in the async functions that don't need to know what the type of the disposable is so we can avoid duplicating functions like InvokeAsync

The specific changes to fix the issue reported are

  1. change InternalExecuteNonQueryAsync to use an explicity context object
  2. make disposable idempotent by including a disposed flag. Previously it was not possible to reach disposable from more than one path so knowledge of disposal was implicit, now it is explicit.
  3. call context dispose from catch blocks ensuring that the registration is correctly cleaned up on all paths

/cc @olegkv

@codecov
Copy link

codecov bot commented Aug 4, 2022

Codecov Report

Base: 71.60% // Head: 71.00% // Decreases project coverage by -0.60% ⚠️

Coverage data is based on head (2a3996a) compared to base (f62ac3b).
Patch coverage: 97.02% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1692      +/-   ##
==========================================
- Coverage   71.60%   71.00%   -0.61%     
==========================================
  Files         293      293              
  Lines       61381    64971    +3590     
==========================================
+ Hits        43953    46133    +2180     
- Misses      17428    18838    +1410     
Flag Coverage Δ
addons 92.38% <ø> (ø)
netcore 74.98% <96.73%> (-0.30%) ⬇️
netfx 68.83% <100.00%> (-0.50%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...core/src/Microsoft/Data/SqlClient/SqlDataReader.cs 72.79% <92.30%> (-0.01%) ⬇️
.../src/Microsoft/Data/SqlClient/AAsyncCallContext.cs 91.42% <95.45%> (+3.92%) ⬆️
...netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs 83.79% <100.00%> (+0.14%) ⬆️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs 81.65% <100.00%> (-1.70%) ⬇️
...etfx/src/Microsoft/Data/SqlClient/SqlDataReader.cs 67.88% <100.00%> (-0.52%) ⬇️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs 81.87% <100.00%> (-0.08%) ⬇️
...osoft/Data/SqlClient/SqlConnectionStringBuilder.cs 88.60% <100.00%> (+0.01%) ⬆️
...ata/SqlClient/SqlConnectionTimeoutErrorInternal.cs 41.96% <0.00%> (-18.75%) ⬇️
...Microsoft/Data/SqlClient/TdsParserHelperClasses.cs 53.38% <0.00%> (-7.41%) ⬇️
...nt/src/Microsoft/Data/ProviderBase/TimeoutTimer.cs 78.57% <0.00%> (-3.58%) ⬇️
... and 23 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@JRahnama JRahnama added this to In Progress in SqlClient v5.1 via automation Aug 10, 2022
@JRahnama JRahnama added this to the 5.1.0-preview1 milestone Aug 10, 2022
@lcheunglci
Copy link
Contributor

lcheunglci commented Oct 4, 2022

@Wraith2 Looks like there's a merge conflict. Let us know if you would like us to resolve it for you.

@David-Engel
Copy link
Contributor

Looks like there's a merge conflict. Let us know if you would like us to resolve it for you.

I took care of it. It was a small change from #1781.

@David-Engel
Copy link
Contributor

That'll teach me to use the GitHub conflict UI. I broke it. I'm looking at it.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Oct 5, 2022

Rebased, checked it still makes sense and addressed the feedback above.

SqlClient v5.1 automation moved this from In Progress to Review Approved Oct 5, 2022
@JRahnama JRahnama merged commit 1cda345 into dotnet:main Oct 5, 2022
SqlClient v5.1 automation moved this from Review Approved to Done Oct 5, 2022
@Wraith2 Wraith2 deleted the perf-nonqueryasync branch October 5, 2022 20:07
@lcheunglci lcheunglci mentioned this pull request Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Memory leak when an exception is raised during execution of SqlCommand with CancellationToken
5 participants