-
Notifications
You must be signed in to change notification settings - Fork 280
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
cache all appcontext switches #2227
cache all appcontext switches #2227
Conversation
@@ -15,8 +15,8 @@ internal static partial class LocalAppContextSwitches | |||
|
|||
private static bool? s_legacyRowVersionNullBehavior; | |||
private static bool? s_suppressInsecureTLSWarning; | |||
private static bool s_makeReadAsyncBlocking; | |||
private static bool s_useMinimumLoginTimeout; | |||
private static bool? s_makeReadAsyncBlocking; |
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.
bool? is a structure with two fields. It is not updated atomically. The lock-free reads and writes below have race conditions.
If you want the reads to be lock-free, you can either:
- Change the type of these caches to
bool
and initialize them eagerly in a static constructors
or - Change the type of these caches to atomically updated integer with 3 possible values uninitialized/false/true. Here is an example of similar pattern in dotnet/runtime: https://github.com/dotnet/runtime/blob/b88c74a109903de213b049177cbc0481a689477f/src/libraries/System.Private.CoreLib/src/System/Globalization/TextInfo.cs#L22C22-L27
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.
Thanks. I've updated all the variables to use the tristate enum pattern.
Failing tests:
|
Thanks. Fixed and added note to tests and source to indicate that private reflection is being used. I wrote that test so I should have remembered. |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs
Outdated
Show resolved
Hide resolved
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2227 +/- ##
==========================================
- Coverage 71.49% 68.34% -3.16%
==========================================
Files 309 309
Lines 62181 62195 +14
==========================================
- Hits 44459 42506 -1953
- Misses 17722 19689 +1967
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
fixes #2225
Change any uncached appcontext switches to use caching like the others.
/cc @JulianRooze