Don't capture AsyncLocals into SQL global timers #26065

benaadams · 2017-12-26T23:14:15Z

Contributes to https://github.com/dotnet/corefx/issues/26064

benaadams · 2017-12-27T00:39:01Z

OSX

Output: Discovering: System.ComponentModel.Composition.Tests
Output: Discovered:  System.ComponentModel.Composition.Tests
Output: Starting:    System.ComponentModel.Composition.Tests
Output: /Users/dotnet-bot/dotnetbuild/work/a0f1062e-868d-4cbd-960e-402ec086e2f6/Work/dcdfb836-f373-47f7-8aa3-dc3477392788/Unzip/RunTests.sh: line 87: 44414 Segmentation fault: 11  (core dumped) $RUNTIME_PATH/dotnet xunit.console.netcore.exe 
   System.ComponentModel.Composition.Tests.dll -xml testResults.xml -notrait Benchmark=true -notrait category=nonnetcoreapptests -notrait category=nonosxtests -notrait category=OuterLoop -notrait category=failing
Output: Traceback (most recent call last):
Output:   File "/Users/dotnet-bot/.dumpling/dumpling.py", line 33, in <module>
Output:     import psutil
Output: ImportError: No module named psutil
Output: Trying to find crash dumps for project: System.ComponentModel.Composition.Tests
Output: Uploading dump file: /Users/dotnet-bot/dotnetbuild/work/a0f1062e-868d-4cbd-960e-402ec086e2f6/Work/dcdfb836-f373-47f7-8aa3-dc3477392788/Unzip/core.44414
Output: ~/dotnetbuild/work/a0f1062e-868d-4cbd-960e-402ec086e2f6/Work/dcdfb836-f373-47f7-8aa3-dc3477392788/Unzip
Output: Finished running tests. End time=16:05:35. Return value was 139
Exit Code: 139

@dotnet-bot test OSX x64 Debug Build

karelz · 2018-01-18T02:27:25Z

@saurabh500 @corivera can you please take a look?

saurabh500 · 2018-01-18T19:34:43Z

@benaadams The original issue thread was very long. Can you give me a short version of what is going on ?
This PR seems to suppress capturing the AsyncLocals for Timer. Couldn't this problem exist with another type? Based on the PR my understanding is that, any usage of Timer should be wrapped in the pattern you have used in this PR.

Maybe I am missing something here.

saurabh500 · 2018-01-18T19:36:07Z

Thanks @karelz for bumping this thread

benaadams · 2018-01-18T19:58:29Z

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 AsyncLocal.OnValueChanged the were receiving notifications everytime the timer fired saying that the asynclocals had been restored.

benaadams · 2018-01-18T20:07:21Z

Based on the PR my understanding is that, any usage of Timer should be wrapped in the pattern you have used in this PR.

Correct, if its an application-wide timer; rather than a timer related to the local state.

I've opened quite a few PRs to fix it #26075, #26071, #26066, #26065, #26077, #26068

saurabh500 · 2018-01-18T20:10:41Z

@benaadams Thanks for the explanation and context.

saurabh500 · 2018-01-18T20:12:01Z

src/System.Data.SqlClient/src/System/Data/ProviderBase/DbConnectionFactory.cs

+            bool restoreFlow = false;
+            try
+            {
+                if (!ExecutionContext.IsFlowSuppressed())


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

benaadams · 2018-01-23T11:15:57Z

src/System.Data.SqlClient/src/System/Data/ProviderBase/DbConnectionPool.cs

@@ -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);


This is a one shot local timer; doesn't require the change

saurabh500 · 2018-01-26T22:38:32Z

src/System.Data.Common/src/System.Data.Common.csproj

@@ -333,6 +333,7 @@
    <Reference Include="System.Text.Encoding.Extensions" />
    <Reference Include="System.Text.RegularExpressions" />
    <Reference Include="System.Threading" />
+    <Reference Include="System.Threading.Timer" />


Why did you need to add this additional dependency?

saurabh500 · 2018-01-26T22:39:46Z

src/Common/src/System/Data/Common/AdapterUtil.cs

@@ -492,5 +492,32 @@ internal static void SetCurrentTransaction(Transaction transaction)
        {
            Transaction.Current = transaction;
        }
+
+        internal static Timer CreateGlobalTimer(TimerCallback callback, object state, int dueTime, int period)


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

I was assuming I could make use of it for ODBC which has the same issue? #26066

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.

saurabh500 · 2018-01-28T05:26:05Z

@benaadams the changes look good functuonally. Do you know if this can impact performance in any way?

benaadams · 2018-01-28T10:16:27Z

test NETFX x86 Release Build

benaadams · 2018-01-28T10:18:26Z

Do you know if this can impact performance in any way?

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

benaadams · 2018-01-28T10:21:22Z

Same netfx SQLClient tests were failing in #26612 so might be infrastructure?

benaadams · 2018-01-28T11:32:27Z

02:41:43       System.Data.SqlClient.Tests.DiagnosticTest.ExecuteNonQueryTest [FAIL]
02:41:43         System.Diagnostics.RemoteExecutorTestBase+RemoteInvokeHandle+RemoteExecutionException : Remote process failed with an unhandled exception.
02:41:43         Stack Trace:
02:41:43           
02:41:43           Child exception:
02:41:43             Xunit.Sdk.TrueException: Assert.True() Failure
02:41:43           Expected: True
02:41:43           Actual:   False
02:41:43              at System.Data.SqlClient.Tests.DiagnosticTest.CollectStatisticsDiagnostics(Action`1 sqlOperation, Boolean enableServerLogging, String methodName)
02:41:43              at System.Data.SqlClient.Tests.DiagnosticTest.<>c.<ExecuteNonQueryTest>b__5_0()
02:41:43           
02:41:43           Child process:
02:41:43             System.Data.SqlClient.Tests, Version=4.3.1.0, Culture=neutral, PublicKeyToken=9d77cc7ad39b68eb System.Data.SqlClient.Tests.DiagnosticTest+<>c Int32 <ExecuteNonQueryTest>b__5_0()
02:41:43           
02:41:43           
02:41:43   Test: ExecuteNonQueryAsyncTest Enabled Listeners

02:41:48     Discovering: System.Data.Common.Tests
02:41:48         System.Diagnostics.RemoteExecutorTestBase+RemoteInvokeHandle+RemoteExecutionException : Remote process failed with an unhandled exception.
02:41:48         Stack Trace:
02:41:48           
02:41:48           Child exception:
02:41:48             Xunit.Sdk.TrueException: Assert.True() Failure
02:41:48           Expected: True
02:41:48           Actual:   False
02:41:48              at System.Data.SqlClient.Tests.DiagnosticTest.<CollectStatisticsDiagnosticsAsync>d__25.MoveNext()
02:41:48           --- End of stack trace from previous location where exception was thrown ---
02:41:48              at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
02:41:48              at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
02:41:48              at System.Data.SqlClient.Tests.DiagnosticTest.<>c.<ExecuteNonQueryAsyncTest>b__14_0()
02:41:48           
02:41:48           Child process:
02:41:48             System.Data.SqlClient.Tests, Version=4.3.1.0, Culture=neutral, PublicKeyToken=9d77cc7ad39b68eb System.Data.SqlClient.Tests.DiagnosticTest+<>c Int32 <ExecuteNonQueryAsyncTest>b__14_0()

etc

benaadams · 2018-01-28T16:13:49Z

NETFX x86 failures https://github.com/dotnet/corefx/issues/26615

stephentoub · 2018-01-29T14:28:42Z

src/Common/src/System/Data/Common/AdapterUtil.Drivers.cs

+    internal static partial class ADP
+    {
+
+        internal static Timer CreateGlobalTimer(TimerCallback callback, object state, int dueTime, int period)


Nit: I would call it UnsafeCreateTimer. Then it can be replaced by Timer.UnsafeCreate if/when it's added.

stephentoub · 2018-01-29T14:29:15Z

src/Common/src/System/Data/Common/AdapterUtil.Drivers.cs

+            // Don't capture the current ExecutionContext and its AsyncLocals onto 
+            // a global timer causing them to live forever
+
+            Timer timer;


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.

stephentoub · 2018-01-29T14:30:32Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SqlDependencyListener.cs

                        _retryTimer = null;
+                        retryTimer?.Dispose();


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(); }

stephentoub · 2018-01-30T12:46:06Z

@dotnet-bot test NETFX x86 Release Build please

* Don't capture AsyncLocals into SQL global timers * Feedback Commit migrated from dotnet/corefx@093126c

benaadams mentioned this pull request Dec 27, 2017

Activity shouldn't capture AsyncLocals into its Timer #26071

Merged

benaadams force-pushed the sqlconn-pool branch from afba626 to a9ba1e2 Compare December 27, 2017 17:23

karelz added the area-System.Data.SqlClient label Dec 27, 2017

karelz assigned benaadams, saurabh500 and corivera Dec 27, 2017

benaadams force-pushed the sqlconn-pool branch from a9ba1e2 to c578e30 Compare December 27, 2017 21:31

saurabh500 reviewed Jan 18, 2018

View reviewed changes

benaadams force-pushed the sqlconn-pool branch from c578e30 to 96d9ce5 Compare January 23, 2018 11:14

benaadams changed the title ~~Don't capture AsyncLocals into SQL ConnPool~~ Don't capture AsyncLocals into SQL global timers Jan 23, 2018

benaadams commented Jan 23, 2018

View reviewed changes

benaadams force-pushed the sqlconn-pool branch from 96d9ce5 to 77248dd Compare January 23, 2018 20:52

saurabh500 reviewed Jan 26, 2018

View reviewed changes

saurabh500 mentioned this pull request Jan 27, 2018

Don't capture AsyncLocals into ODBC ConnPool #26066

Merged

Don't capture AsyncLocals into SQL global timers

2891f51

benaadams force-pushed the sqlconn-pool branch from 7721da9 to 2891f51 Compare January 27, 2018 21:41

stephentoub reviewed Jan 29, 2018

View reviewed changes

stephentoub approved these changes Jan 29, 2018

View reviewed changes

saurabh500 approved these changes Jan 29, 2018

View reviewed changes

Feedback

0ace972

stephentoub merged commit 093126c into dotnet:master Jan 30, 2018

benaadams deleted the sqlconn-pool branch January 30, 2018 15:19

karelz added this to the 2.1.0 milestone Feb 4, 2018

rjvdboon mentioned this pull request May 17, 2018

Unhandled Exceptions in Threading mono/mono#8716

Closed

benaadams mentioned this pull request Jan 31, 2020

[API] Timer.UnsafeCreate dotnet/runtime#24770

Closed

saurabh500 mentioned this pull request Jan 31, 2020

Consolidate the connection pool code at one location dotnet/runtime#24808

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Don't capture AsyncLocals into SQL global timers (dotnet/corefx#26065)

819c914

* Don't capture AsyncLocals into SQL global timers * Feedback Commit migrated from dotnet/corefx@093126c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't capture AsyncLocals into SQL global timers #26065

Don't capture AsyncLocals into SQL global timers #26065

benaadams commented Dec 26, 2017

benaadams commented Dec 27, 2017

karelz commented Jan 18, 2018

saurabh500 commented Jan 18, 2018

saurabh500 commented Jan 18, 2018

benaadams commented Jan 18, 2018 •

edited

Loading

benaadams commented Jan 18, 2018

saurabh500 commented Jan 18, 2018

saurabh500 Jan 18, 2018

benaadams Jan 23, 2018

benaadams Jan 23, 2018

saurabh500 Jan 26, 2018

saurabh500 Jan 26, 2018

benaadams Jan 26, 2018

saurabh500 Jan 26, 2018

benaadams Jan 27, 2018

saurabh500 commented Jan 28, 2018

benaadams commented Jan 28, 2018

benaadams commented Jan 28, 2018

benaadams commented Jan 28, 2018

benaadams commented Jan 28, 2018

benaadams commented Jan 28, 2018 •

edited

Loading

stephentoub Jan 29, 2018

benaadams Jan 29, 2018

stephentoub Jan 29, 2018

benaadams Jan 29, 2018

stephentoub Jan 29, 2018

benaadams Jan 29, 2018

stephentoub commented Jan 30, 2018

		@@ -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);

Don't capture AsyncLocals into SQL global timers #26065

Don't capture AsyncLocals into SQL global timers #26065

Conversation

benaadams commented Dec 26, 2017

benaadams commented Dec 27, 2017

karelz commented Jan 18, 2018

saurabh500 commented Jan 18, 2018

saurabh500 commented Jan 18, 2018

benaadams commented Jan 18, 2018 • edited Loading

benaadams commented Jan 18, 2018

saurabh500 commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saurabh500 commented Jan 28, 2018

benaadams commented Jan 28, 2018

benaadams commented Jan 28, 2018

benaadams commented Jan 28, 2018

benaadams commented Jan 28, 2018

benaadams commented Jan 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub commented Jan 30, 2018

benaadams commented Jan 18, 2018 •

edited

Loading

benaadams commented Jan 28, 2018 •

edited

Loading