From 967a9cb20bc3443cd5590fbd5d073389e5a5d0f9 Mon Sep 17 00:00:00 2001 From: Lawrence LCI Date: Tue, 26 Apr 2022 15:43:20 -0700 Subject: [PATCH 1/5] Merge changes from netfx into netcore for TdsParserSessionPool --- .../Data/SqlClient/TdsParserSessionPool.cs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs index be3cd1404d..3636300957 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs @@ -89,6 +89,25 @@ internal void Deactivate() } } +#if NETFRAMEWORK + // This is called from a ThreadAbort - ensure that it can be run from a CER Catch + internal void BestEffortCleanup() + { + for (int i = 0; i < _cache.Count; i++) + { + TdsParserStateObject session = _cache[i]; + if (null != session) + { + var sessionHandle = session.Handle; + if (sessionHandle != null) + { + sessionHandle.Dispose(); + } + } + } + } +#endif + internal void Dispose() { SqlClientEventSource.Log.TryAdvancedTraceEvent(" {0} disposing cachedCount={1}", ObjectID, _cachedCount); From f7d8eafc9d31792b027f61d298e0b046cefce807 Mon Sep 17 00:00:00 2001 From: Lawrence LCI Date: Tue, 26 Apr 2022 16:06:33 -0700 Subject: [PATCH 2/5] Move the TdsParserSessionPool to the shared common, and updated reference in the netcore project --- .../netcore/src/Microsoft.Data.SqlClient.csproj | 4 +++- .../src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs | 0 2 files changed, 3 insertions(+), 1 deletion(-) rename src/Microsoft.Data.SqlClient/{netcore => }/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs (100%) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj index 70fe79041e..d46486469c 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj @@ -478,6 +478,9 @@ Microsoft\Data\SqlClient\TdsRecordBufferSetter.cs + + Microsoft\Data\SqlClient\TdsParserSessionPool.cs + Microsoft\Data\SqlClient\TdsValueSetter.cs @@ -646,7 +649,6 @@ - diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs similarity index 100% rename from src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs rename to src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs From 505a61178534fc0723dece1c5729a3f1fe8ec6fc Mon Sep 17 00:00:00 2001 From: Lawrence LCI Date: Tue, 26 Apr 2022 16:20:51 -0700 Subject: [PATCH 3/5] Update the reference to the TdsParserSessionPool in netfx project and remove the netfx version of the file --- .../netfx/src/Microsoft.Data.SqlClient.csproj | 4 +- .../Data/SqlClient/TdsParserSessionPool.cs | 241 ------------------ .../Data/SqlClient/TdsParserSessionPool.cs | 6 +- 3 files changed, 8 insertions(+), 243 deletions(-) delete mode 100644 src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj index b1e683badc..a032252c55 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj @@ -542,6 +542,9 @@ Microsoft\Data\SqlClient\TdsRecordBufferSetter.cs + + Microsoft\Data\SqlClient\TdsParserSessionPool.cs + Microsoft\Data\SqlClient\TdsValueSetter.cs @@ -632,7 +635,6 @@ - diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs deleted file mode 100644 index 5c1f64aa09..0000000000 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs +++ /dev/null @@ -1,241 +0,0 @@ -// 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; -using System.Collections.Generic; -using System.Diagnostics; -using Microsoft.Data.Common; - -namespace Microsoft.Data.SqlClient -{ - internal class TdsParserSessionPool - { - // NOTE: This is a very simplistic, lightweight pooler. It wasn't - // intended to handle huge number of items, just to keep track - // of the session objects to ensure that they're cleaned up in - // a timely manner, to avoid holding on to an unacceptible - // amount of server-side resources in the event that consumers - // let their data readers be GC'd, instead of explicitly - // closing or disposing of them - - private const int MaxInactiveCount = 10; // pick something, preferably small... - - private static int _objectTypeCount; // EventSource Counter - private readonly int _objectID = System.Threading.Interlocked.Increment(ref _objectTypeCount); - - private readonly TdsParser _parser; // parser that owns us - private readonly List _cache; // collection of all known sessions - private int _cachedCount; // lock-free _cache.Count - private TdsParserStateObject[] _freeStateObjects; // collection of all sessions available for reuse - private int _freeStateObjectCount; // Number of available free sessions - - internal TdsParserSessionPool(TdsParser parser) - { - _parser = parser; - _cache = new List(); - _freeStateObjects = new TdsParserStateObject[MaxInactiveCount]; - _freeStateObjectCount = 0; - SqlClientEventSource.Log.TryAdvancedTraceEvent(" {0} created session pool for parser {1}", ObjectID, parser.ObjectID); - } - - private bool IsDisposed - { - get - { - return (null == _freeStateObjects); - } - } - - internal int ObjectID - { - get - { - return _objectID; - } - } - - internal void Deactivate() - { - // When being deactivated, we check all the sessions in the - // cache to make sure they're cleaned up and then we dispose of - // sessions that are past what we want to keep around. - using (TryEventScope.Create(" {0} deactivating cachedCount={1}", ObjectID, _cachedCount)) - { - lock (_cache) - { - // NOTE: The PutSession call below may choose to remove the - // session from the cache, which will throw off our - // enumerator. We avoid that by simply indexing backward - // through the array. - - for (int i = _cache.Count - 1; i >= 0; i--) - { - TdsParserStateObject session = _cache[i]; - - if (null != session) - { - if (session.IsOrphaned) - { - // TODO: consider adding a performance counter for the number of sessions we reclaim - SqlClientEventSource.Log.TryAdvancedTraceEvent(" {0} reclaiming session {1}", ObjectID, session.ObjectID); - PutSession(session); - } - } - } - // TODO: re-enable this assert when the connection isn't doomed. - //Debug.Assert (_cachedCount < MaxInactiveCount, "non-orphaned connection past initial allocation?"); - } - } - } - - // This is called from a ThreadAbort - ensure that it can be run from a CER Catch - internal void BestEffortCleanup() - { - for (int i = 0; i < _cache.Count; i++) - { - TdsParserStateObject session = _cache[i]; - if (null != session) - { - var sessionHandle = session.Handle; - if (sessionHandle != null) - { - sessionHandle.Dispose(); - } - } - } - } - - internal void Dispose() - { - SqlClientEventSource.Log.TryAdvancedTraceEvent(" {0} disposing cachedCount={1}", ObjectID, _cachedCount); - lock (_cache) - { - // Dispose free sessions - for (int i = 0; i < _freeStateObjectCount; i++) - { - if (_freeStateObjects[i] != null) - { - _freeStateObjects[i].Dispose(); - } - } - _freeStateObjects = null; - _freeStateObjectCount = 0; - - // Dispose orphaned sessions - for (int i = 0; i < _cache.Count; i++) - { - if (_cache[i] != null) - { - if (_cache[i].IsOrphaned) - { - _cache[i].Dispose(); - } - else - { - // Remove the "initial" callback (this will allow the stateObj to be GC collected if need be) - _cache[i].DecrementPendingCallbacks(false); - } - } - } - _cache.Clear(); - _cachedCount = 0; - - // Any active sessions will take care of themselves - // (It's too dangerous to dispose them, as this can cause AVs) - } - } - - internal TdsParserStateObject GetSession(object owner) - { - TdsParserStateObject session; - lock (_cache) - { - if (IsDisposed) - { - throw ADP.ClosedConnectionError(); - } - else if (_freeStateObjectCount > 0) - { - // Free state object - grab it - _freeStateObjectCount--; - session = _freeStateObjects[_freeStateObjectCount]; - _freeStateObjects[_freeStateObjectCount] = null; - Debug.Assert(session != null, "There was a null session in the free session list?"); - } - else - { - // No free objects, create a new one - session = _parser.CreateSession(); - SqlClientEventSource.Log.TryAdvancedTraceEvent(" {0} adding session {1} to pool", ObjectID, session.ObjectID); - - _cache.Add(session); - _cachedCount = _cache.Count; - } - - session.Activate(owner); - } - SqlClientEventSource.Log.TryAdvancedTraceEvent(" {0} using session {1}", ObjectID, session.ObjectID); - return session; - } - - internal void PutSession(TdsParserStateObject session) - { - Debug.Assert(null != session, "null session?"); - //Debug.Assert(null != session.Owner, "session without owner?"); - - bool okToReuse = session.Deactivate(); - - lock (_cache) - { - if (IsDisposed) - { - // We're diposed - just clean out the session - Debug.Assert(_cachedCount == 0, "SessionPool is disposed, but there are still sessions in the cache?"); - session.Dispose(); - } - else if ((okToReuse) && (_freeStateObjectCount < MaxInactiveCount)) - { - // Session is good to re-use and our cache has space - SqlClientEventSource.Log.TryAdvancedTraceEvent(" {0} keeping session {1} cachedCount={2}", ObjectID, session.ObjectID, _cachedCount); - Debug.Assert(!session._pendingData, "pending data on a pooled session?"); - - _freeStateObjects[_freeStateObjectCount] = session; - _freeStateObjectCount++; - } - else - { - // Either the session is bad, or we have no cache space - so dispose the session and remove it - SqlClientEventSource.Log.TryAdvancedTraceEvent(" {0} disposing session {1} cachedCount={2}", ObjectID, session.ObjectID, _cachedCount); - - bool removed = _cache.Remove(session); - Debug.Assert(removed, "session not in pool?"); - _cachedCount = _cache.Count; - session.Dispose(); - } - - session.RemoveOwner(); - } - } - - internal string TraceString() - { - return String.Format(/*IFormatProvider*/ null, - "(ObjID={0}, free={1}, cached={2}, total={3})", - _objectID, - null == _freeStateObjects ? "(null)" : _freeStateObjectCount.ToString((IFormatProvider)null), - _cachedCount, - _cache.Count); - } - - internal int ActiveSessionsCount - { - get - { - return _cachedCount - _freeStateObjectCount; - } - } - } -} - - diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs index 3636300957..4b75361eb9 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs @@ -200,8 +200,12 @@ internal void PutSession(TdsParserStateObject session) { // Session is good to re-use and our cache has space SqlClientEventSource.Log.TryAdvancedTraceEvent(" {0} keeping session {1} cachedCount={2}", ObjectID, session.ObjectID, _cachedCount); + // TODO: To avoid merge conflict with TdsParserStateObject in PR# 1520, i.e. it'll be merged in the common code base, we can clean this up in a later PR. +#if NETFRAMEWORK + Debug.Assert(!session._pendingData, "pending data on a pooled session?"); +#else Debug.Assert(!session.HasPendingData, "pending data on a pooled session?"); - +#endif _freeStateObjects[_freeStateObjectCount] = session; _freeStateObjectCount++; } From e543060211b99f473073512735fe4e4f47618732 Mon Sep 17 00:00:00 2001 From: Lawrence LCI Date: Tue, 26 Apr 2022 16:24:04 -0700 Subject: [PATCH 4/5] Fix coding style resolving IDE0008 and IDE1006 --- .../src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs index 4b75361eb9..3dd9649ee2 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs @@ -21,8 +21,8 @@ internal class TdsParserSessionPool private const int MaxInactiveCount = 10; // pick something, preferably small... - private static int _objectTypeCount; // EventSource Counter - private readonly int _objectID = System.Threading.Interlocked.Increment(ref _objectTypeCount); + private static int s_objectTypeCount; // EventSource Counter + private readonly int _objectID = System.Threading.Interlocked.Increment(ref s_objectTypeCount); private readonly TdsParser _parser; // parser that owns us private readonly List _cache; // collection of all known sessions @@ -98,7 +98,7 @@ internal void BestEffortCleanup() TdsParserStateObject session = _cache[i]; if (null != session) { - var sessionHandle = session.Handle; + SNIHandle sessionHandle = session.Handle; if (sessionHandle != null) { sessionHandle.Dispose(); From db151f1cae9175cfb4dcd5ed5929e76829debd5f Mon Sep 17 00:00:00 2001 From: Lawrence LCI Date: Mon, 3 Oct 2022 10:45:14 -0700 Subject: [PATCH 5/5] Address comments in the code review --- .../Data/SqlClient/TdsParserSessionPool.cs | 47 ++++++++----------- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs index 3dd9649ee2..ed53a831c4 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserSessionPool.cs @@ -89,25 +89,6 @@ internal void Deactivate() } } -#if NETFRAMEWORK - // This is called from a ThreadAbort - ensure that it can be run from a CER Catch - internal void BestEffortCleanup() - { - for (int i = 0; i < _cache.Count; i++) - { - TdsParserStateObject session = _cache[i]; - if (null != session) - { - SNIHandle sessionHandle = session.Handle; - if (sessionHandle != null) - { - sessionHandle.Dispose(); - } - } - } - } -#endif - internal void Dispose() { SqlClientEventSource.Log.TryAdvancedTraceEvent(" {0} disposing cachedCount={1}", ObjectID, _cachedCount); @@ -200,7 +181,6 @@ internal void PutSession(TdsParserStateObject session) { // Session is good to re-use and our cache has space SqlClientEventSource.Log.TryAdvancedTraceEvent(" {0} keeping session {1} cachedCount={2}", ObjectID, session.ObjectID, _cachedCount); - // TODO: To avoid merge conflict with TdsParserStateObject in PR# 1520, i.e. it'll be merged in the common code base, we can clean this up in a later PR. #if NETFRAMEWORK Debug.Assert(!session._pendingData, "pending data on a pooled session?"); #else @@ -225,13 +205,7 @@ internal void PutSession(TdsParserStateObject session) } - internal int ActiveSessionsCount - { - get - { - return _cachedCount - _freeStateObjectCount; - } - } + internal int ActiveSessionsCount => _cachedCount - _freeStateObjectCount; internal string TraceString() { @@ -242,6 +216,25 @@ internal string TraceString() _cachedCount, _cache.Count); } + +#if NETFRAMEWORK + // This is called from a ThreadAbort - ensure that it can be run from a CER Catch + internal void BestEffortCleanup() + { + for (int i = 0; i < _cache.Count; i++) + { + TdsParserStateObject session = _cache[i]; + if (null != session) + { + SNIHandle sessionHandle = session.Handle; + if (sessionHandle != null) + { + sessionHandle.Dispose(); + } + } + } + } +#endif } }