Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Don't capture AsyncLocals into SQL global timers #26065

Merged
merged 2 commits into from
Jan 30, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions src/Common/src/System/Data/Common/AdapterUtil.Drivers.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Threading;

namespace System.Data.Common
{
internal static partial class ADP
{

internal static Timer CreateGlobalTimer(TimerCallback callback, object state, int dueTime, int period)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed

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

Timer timer;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed

bool restoreFlow = false;
try
{
if (!ExecutionContext.IsFlowSuppressed())
{
ExecutionContext.SuppressFlow();
restoreFlow = true;
}

timer = new Timer(callback, state, dueTime, period);
}
finally
{
// Restore the current ExecutionContext
if (restoreFlow)
ExecutionContext.RestoreFlow();
}

return timer;
}
}
}
3 changes: 3 additions & 0 deletions src/System.Data.SqlClient/src/System.Data.SqlClient.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@
<Compile Include="$(CommonPath)\System\Data\Common\AdapterUtil.cs">
<Link>System\Data\Common\AdapterUtil.cs</Link>
</Compile>
<Compile Include="$(CommonPath)\System\Data\Common\AdapterUtil.Drivers.cs">
<Link>System\Data\Common\AdapterUtil.Drivers.cs</Link>
</Compile>
<Compile Include="System\Data\Common\AdapterUtil.SqlClient.cs" />
<Compile Include="System\Data\Common\SR.cs" />
<Compile Include="System\Data\Common\DbConnectionOptions.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,11 +123,12 @@ virtual internal DbConnectionPoolGroupProviderInfo CreateConnectionPoolGroupProv
return null;
}

private Timer CreatePruningTimer()
{
TimerCallback callback = new TimerCallback(PruneConnectionPoolGroups);
return new Timer(callback, null, PruningDueTime, PruningPeriod);
}
private Timer CreatePruningTimer() =>
ADP.CreateGlobalTimer(
new TimerCallback(PruneConnectionPoolGroups),
null,
PruningDueTime,
PruningPeriod);

protected DbConnectionOptions FindConnectionOptions(DbConnectionPoolKey key)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -664,10 +664,12 @@ internal void Clear()
ReclaimEmancipatedObjects();
}

private Timer CreateCleanupTimer()
{
return (new Timer(new TimerCallback(this.CleanupCallback), null, _cleanupWait, _cleanupWait));
}
private Timer CreateCleanupTimer() =>
ADP.CreateGlobalTimer(
new TimerCallback(CleanupCallback),
null,
_cleanupWait,
_cleanupWait);

private DbConnectionInternal CreateObject(DbConnection owningObject, DbConnectionOptions userOptions, DbConnectionInternal oldConnection)
{
Expand Down Expand Up @@ -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);
Copy link
Member Author

Choose a reason for hiding this comment

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

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


bool timerIsNotDisposed;
try { }
finally
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -612,7 +612,9 @@ private void Restart(object unused)
_timeoutParam.Value = _defaultWaitforTimeout; // If success, reset to default for re-queue.
AsynchronouslyQueryServiceBrokerQueue();
_errorState = false;
Timer retryTimer = _retryTimer;
_retryTimer = null;
retryTimer?.Dispose();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed

}
}

Expand Down Expand Up @@ -1470,4 +1472,4 @@ internal bool Stop(

return stopped;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,11 @@ private SqlDependencyPerAppDomainDispatcher()
_notificationIdToDependenciesHash = new Dictionary<string, DependencyList>();
_commandHashToNotificationId = new Dictionary<string, string>();

_timeoutTimer = new Timer(new TimerCallback(TimeoutTimerCallback), null, Timeout.Infinite, Timeout.Infinite);
_timeoutTimer = ADP.CreateGlobalTimer(
new TimerCallback(TimeoutTimerCallback),
null,
Timeout.Infinite,
Timeout.Infinite);

SubscribeToAppDomainUnload();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2295,7 +2295,11 @@ internal void ReadSni(TaskCompletionSource<object> completion)

if (_networkPacketTimeout == null)
{
_networkPacketTimeout = new Timer(OnTimeout, null, Timeout.Infinite, Timeout.Infinite);
_networkPacketTimeout = ADP.CreateGlobalTimer(
new TimerCallback(OnTimeout),
null,
Timeout.Infinite,
Timeout.Infinite);
}

// -1 == Infinite
Expand Down