Don't capture AsyncLocals into SQL global timers#26065
Don't capture AsyncLocals into SQL global timers#26065stephentoub merged 2 commits intodotnet:masterfrom
Conversation
|
OSX @dotnet-bot test OSX x64 Debug Build |
afba626 to
a9ba1e2
Compare
a9ba1e2 to
c578e30
Compare
|
@saurabh500 @corivera can you please take a look? |
|
@benaadams The original issue thread was very long. Can you give me a short version of what is going on ? Maybe I am missing something here. |
|
Thanks @karelz for bumping this thread |
|
Timer captures asynclocals; so when it runs it runs with the same execution context of the call that set up the timer; which is likely a desirable default state. However; the pool timers are lazily set up on first time use of the a db connection (also good); but they are application wide pools, so the shouldn't capture the asynclocals of the first call making a db connection. The issue in https://github.com/dotnet/corefx/issues/25477#issuecomment-346866897 was that the authenticated user was stored in an asynclocal; so it was restored to the execution context every time the timer fired (not desirable). Its not a security issue as the methods the timer calls don't touch the asynclocals and are isolated; however it will both prevent them from being GC'd and as in the issue they were subscribed to |
|
@benaadams Thanks for the explanation and context. |
| bool restoreFlow = false; | ||
| try | ||
| { | ||
| if (!ExecutionContext.IsFlowSuppressed()) |
There was a problem hiding this comment.
This code/pattern is being used at multiple places. Can this be refactored into a reusable method. AdapterUtil.cs is a good place to put such utilities in System.Data
c578e30 to
96d9ce5
Compare
| @@ -728,6 +730,7 @@ private DbConnectionInternal CreateObject(DbConnection owningObject, DbConnectio | |||
|
|
|||
| // timer allocation has to be done out of CER block | |||
| Timer t = new Timer(new TimerCallback(this.ErrorCallback), null, Timeout.Infinite, Timeout.Infinite); | |||
There was a problem hiding this comment.
This is a one shot local timer; doesn't require the change
96d9ce5 to
77248dd
Compare
| <Reference Include="System.Text.Encoding.Extensions" /> | ||
| <Reference Include="System.Text.RegularExpressions" /> | ||
| <Reference Include="System.Threading" /> | ||
| <Reference Include="System.Threading.Timer" /> |
There was a problem hiding this comment.
Why did you need to add this additional dependency?
| Transaction.Current = transaction; | ||
| } | ||
|
|
||
| internal static Timer CreateGlobalTimer(TimerCallback callback, object state, int dueTime, int period) |
There was a problem hiding this comment.
In case this change is causing you to add an additional dependency in System.Data.Common, it would make sense to move the change to System.Data.SqlClient\src\System\Data\Common\AdapterUtil.SqlClient.cs
There was a problem hiding this comment.
I was assuming I could make use of it for ODBC which has the same issue? #26066
There was a problem hiding this comment.
Yeah, the code should be shared between the two PRs.
In this case you can create a src/Common/src/System/Data/Common/AdapterUtil.Drivers.cs and add this CreateGlobalTimer function to a partial ADP class. Then you could include the reference to the file in System.Data.SqlClient and S.D.Odbc. This way the function doesn't need to be compiled into S.D.Common.
The DbConnectionPool code that is duplicated in System/Data/ProviderBase of S.D.SqlClient and S.D.Odbc should be moved to a single code as well.
A little bit of History
What is happening in .Net framework is that S.D.Odbc and S.D.SqlClient reside in System.Data.dll
While moving these assemblies to .Net core, the connection pool code got duplicated. If there was a single set of files for connection pool, then a single PR would have taken care of this issue for both Odbc and SqlClient.
Consolidation of connection pool code is outside the scope of this PR. I will open an issue to consolidate the code for Connection Pool.
7721da9 to
2891f51
Compare
|
@benaadams the changes look good functuonally. Do you know if this can impact performance in any way? |
|
test NETFX x86 Release Build |
Shouldn't greatly; the setup only happens once, and the periodic trigger of the timer will be faster as it won't be restoring and undoing the ExecutionContext - but that is pretty fast |
|
Same netfx SQLClient tests were failing in #26612 so might be infrastructure? |
etc |
|
NETFX x86 failures https://github.com/dotnet/corefx/issues/26615 |
| internal static partial class ADP | ||
| { | ||
|
|
||
| internal static Timer CreateGlobalTimer(TimerCallback callback, object state, int dueTime, int period) |
There was a problem hiding this comment.
Nit: I would call it UnsafeCreateTimer. Then it can be replaced by Timer.UnsafeCreate if/when it's added.
| // Don't capture the current ExecutionContext and its AsyncLocals onto | ||
| // a global timer causing them to live forever | ||
|
|
||
| Timer timer; |
There was a problem hiding this comment.
You can just return it from within the try block rather than declaring it here, initializing it in the try, and then returning it after the finally.
| _errorState = false; | ||
| Timer retryTimer = _retryTimer; | ||
| _retryTimer = null; | ||
| retryTimer?.Dispose(); |
There was a problem hiding this comment.
It's not a big deal, but as long as we're doing the null check, we may as well avoid the unnecessary null write:
Timer retryTimer = _retryTimer;
if (retryTimer != null)
{
_returnTimer = null;
retryTimer.Dispose();
}|
@dotnet-bot test NETFX x86 Release Build please |
* Don't capture AsyncLocals into SQL global timers * Feedback Commit migrated from dotnet/corefx@093126c
Contributes to https://github.com/dotnet/corefx/issues/26064