Skip to content

Bug: fix naming, order and formatting for diagnostics - #1637

Merged
DavoudEshtehari merged 1 commit into
dotnet:mainfrom
Wraith2:order-diagnostics
Jun 7, 2022
Merged

Bug: fix naming, order and formatting for diagnostics#1637
DavoudEshtehari merged 1 commit into
dotnet:mainfrom
Wraith2:order-diagnostics

Conversation

@Wraith2

@Wraith2 Wraith2 commented Jun 5, 2022

Copy link
Copy Markdown
Contributor

Fixes #1634

We were not being consistent about calling diagnostic message output functions before we set the result/exception/cancel methods on the TaskCompletionSource that the users is waiting on. This allowed a rare race condition to occur in libraries consuming diagnostic events and attempting to correlate it to their own current context.

This PR does 3 things:

  1. ensures that all disgnostic message methods in async methods are called before tcs methods.
  2. changes the name of the diagnostic source to be consistent with the other uses in the library and the applivable naming rules, it should be s_diagnosticListener not _diagnosticListener
  3. reformats the task continuation lambdas so it's clearer to understand the scope.

N.B. netfx does not use diagnostics so no changes are required for that part of the project.

/cc @okolvik-avento if possible you could try the output artifacts nuget package once the CI run has completed?

@codecov

codecov Bot commented Jun 5, 2022

Copy link
Copy Markdown

Codecov Report

Merging #1637 (530de53) into main (9b1996a) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1637      +/-   ##
==========================================
+ Coverage   71.30%   71.32%   +0.02%     
==========================================
  Files         295      295              
  Lines       61195    61205      +10     
==========================================
+ Hits        43637    43657      +20     
+ Misses      17558    17548      -10     
Flag Coverage Δ
addons 92.38% <ø> (ø)
netcore 74.84% <100.00%> (-0.06%) ⬇️
netfx 69.24% <ø> (+0.09%) ⬆️

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

Impacted Files Coverage Δ
...netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs 83.64% <100.00%> (+0.05%) ⬆️
...ActiveDirectoryAuthenticationTimeoutRetryHelper.cs 56.81% <0.00%> (-6.82%) ⬇️
...rc/Microsoft/Data/SqlClient/SQLFallbackDNSCache.cs 90.62% <0.00%> (-6.25%) ⬇️
...re/src/Microsoft/Data/SqlClient/SNI/SNINpHandle.cs 73.29% <0.00%> (-5.12%) ⬇️
...osoft/Data/SqlClient/SqlConnectionStringBuilder.cs 87.94% <0.00%> (-0.41%) ⬇️
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs 73.12% <0.00%> (-0.30%) ⬇️
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 73.70% <0.00%> (-0.05%) ⬇️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 70.98% <0.00%> (-0.02%) ⬇️
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs 68.05% <0.00%> (+0.08%) ⬆️
...ient/netfx/src/Microsoft/Data/SqlClient/SqlUtil.cs 58.36% <0.00%> (+0.24%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b1996a...530de53. Read the comment docs.

@okolvik-avento

Copy link
Copy Markdown

@Wraith2 Running tests now

@okolvik-avento

Copy link
Copy Markdown

@Wraith2 Works great! No exceptions after over 500k requests.

@okolvik-avento

Copy link
Copy Markdown

Over 5 million requests now without exceptions

@David-Engel David-Engel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@DavoudEshtehari DavoudEshtehari added this to the 5.0.0-preview3 milestone Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area\Netcore Issues that apply only to .NET runtime or the 'netcore' project folder.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix where diagnostic listener SqlDiagnosticsListener.OnAfterCommand runs to avoid implict concurrent execution

4 participants