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 d1ddbf37af..f5e3590286 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj @@ -343,6 +343,8 @@ + + diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIHandle.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIHandle.cs index 019ecf2b23..6c5bda96b2 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIHandle.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIHandle.cs @@ -40,10 +40,9 @@ internal abstract class SNIHandle /// Send a packet asynchronously /// /// SNI packet - /// /// Completion callback /// SNI error code - public abstract uint SendAsync(SNIPacket packet, bool disposePacketAfterSendAsync, SNIAsyncCallback callback = null); + public abstract uint SendAsync(SNIPacket packet, SNIAsyncCallback callback = null); /// /// Receive a packet synchronously @@ -87,6 +86,11 @@ internal abstract class SNIHandle public abstract Guid ConnectionId { get; } public virtual int ReserveHeaderSize => 0; + + public abstract SNIPacket RentPacket(int headerSize, int dataSize); + + public abstract void ReturnPacket(SNIPacket packet); + #if DEBUG /// /// Test handle for killing underlying connection diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsConnection.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsConnection.cs index a78bdd1aa2..f3ed0c1be0 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsConnection.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsConnection.cs @@ -115,7 +115,7 @@ public uint SendAsync(SNIPacket packet, SNIAsyncCallback callback) { lock (this) { - return _lowerHandle.SendAsync(packet, false, callback); + return _lowerHandle.SendAsync(packet, callback); } } finally @@ -136,7 +136,7 @@ public uint ReceiveAsync(ref SNIPacket packet) { if (packet != null) { - packet.Release(); + ReturnPacket(packet); packet = null; } @@ -188,7 +188,8 @@ public void HandleReceiveError(SNIPacket packet) handle.HandleReceiveError(packet); } } - packet?.Release(); + Debug.Assert(!packet.IsInvalid, "packet was returned by MarsConnection child, child sessions should not release the packet"); + ReturnPacket(packet); } /// @@ -258,7 +259,7 @@ public void HandleReceiveComplete(SNIPacket packet, uint sniErrorCode) _currentHeader.Read(_headerBytes); _dataBytesLeft = (int)_currentHeader.length; - _currentPacket = new SNIPacket(headerSize: 0, dataSize: (int)_currentHeader.length); + _currentPacket = _lowerHandle.RentPacket(headerSize: 0, dataSize: (int)_currentHeader.length); } currentHeader = _currentHeader; @@ -322,6 +323,11 @@ public void HandleReceiveComplete(SNIPacket packet, uint sniErrorCode) { SNICommon.ReportSNIError(SNIProviders.SMUX_PROV, SNICommon.InternalExceptionError, e); } + + Debug.Assert(_currentPacket == currentPacket, "current and _current are not the same"); + ReturnPacket(currentPacket); + currentPacket = null; + _currentPacket = null; } lock (this) @@ -379,6 +385,16 @@ public void DisableSsl() } } + public SNIPacket RentPacket(int headerSize, int dataSize) + { + return _lowerHandle.RentPacket(headerSize, dataSize); + } + + public void ReturnPacket(SNIPacket packet) + { + _lowerHandle.ReturnPacket(packet); + } + #if DEBUG /// /// Test handle for killing underlying connection diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsHandle.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsHandle.cs index 7c740be85c..54572634cd 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsHandle.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsHandle.cs @@ -26,6 +26,7 @@ internal sealed class SNIMarsHandle : SNIHandle private readonly ManualResetEventSlim _packetEvent = new ManualResetEventSlim(false); private readonly ManualResetEventSlim _ackEvent = new ManualResetEventSlim(false); private readonly SNISMUXHeader _currentHeader = new SNISMUXHeader(); + private readonly SNIAsyncCallback _handleSendCompleteCallback; private uint _sendHighwater = 4; private int _asyncReceives = 0; @@ -79,6 +80,7 @@ public SNIMarsHandle(SNIMarsConnection connection, ushort sessionId, object call _sessionId = sessionId; _connection = connection; _callbackObject = callbackObject; + _handleSendCompleteCallback = HandleSendComplete; SendControlPacket(SNISMUXFlags.SMUX_SYN); _status = TdsEnums.SNI_SUCCESS; } @@ -92,7 +94,7 @@ private void SendControlPacket(SNISMUXFlags flags) long scopeID = SqlClientEventSource.Log.SNIScopeEnterEvent(""); try { - SNIPacket packet = new SNIPacket(headerSize: SNISMUXHeader.HEADER_LENGTH, dataSize: 0); + SNIPacket packet = RentPacket(headerSize: SNISMUXHeader.HEADER_LENGTH, dataSize: 0); lock (this) { @@ -102,6 +104,7 @@ private void SendControlPacket(SNISMUXFlags flags) } _connection.Send(packet); + ReturnPacket(packet); } finally { @@ -263,17 +266,16 @@ private uint SendPendingPackets() /// Send a packet asynchronously /// /// SNI packet - /// /// Completion callback /// SNI error code - public override uint SendAsync(SNIPacket packet, bool disposePacketAfterSendAsync, SNIAsyncCallback callback = null) + public override uint SendAsync(SNIPacket packet, SNIAsyncCallback callback = null) { long scopeID = SqlClientEventSource.Log.SNIScopeEnterEvent(""); try { lock (this) { - _sendPacketQueue.Enqueue(new SNIMarsQueuedPacket(packet, callback != null ? callback : HandleSendComplete)); + _sendPacketQueue.Enqueue(new SNIMarsQueuedPacket(packet, callback ?? _handleSendCompleteCallback)); } SendPendingPackets(); @@ -340,13 +342,17 @@ public void HandleReceiveError(SNIPacket packet) long scopeID = SqlClientEventSource.Log.SNIScopeEnterEvent(""); try { + // SNIMarsHandle should only receive calls to this function from the SNIMarsConnection aggregator class + // which should handle ownership of the packet because the individual mars handles are not aware of + // each other and cannot know if they are the last one in the list and that it is safe to return the packet + lock (_receivedPacketQueue) { _connectionError = SNILoadHandle.SingletonInstance.LastError; _packetEvent.Set(); } - ((TdsParserStateObject)_callbackObject).ReadAsyncCallback(PacketHandle.FromManagedPacket(packet), 1); + ((TdsParserStateObject)_callbackObject).ReadAsyncCallback(PacketHandle.FromManagedPacket(packet), 1); } finally { @@ -370,6 +376,7 @@ public void HandleSendComplete(SNIPacket packet, uint sniErrorCode) ((TdsParserStateObject)_callbackObject).WriteAsyncCallback(PacketHandle.FromManagedPacket(packet), sniErrorCode); } + _connection.ReturnPacket(packet); } finally { @@ -432,6 +439,8 @@ public void HandleReceiveComplete(SNIPacket packet, SNISMUXHeader header) ((TdsParserStateObject)_callbackObject).ReadAsyncCallback(PacketHandle.FromManagedPacket(packet), 0); } + + _connection.ReturnPacket(packet); } lock (this) @@ -556,21 +565,14 @@ public override void SetBufferSize(int bufferSize) { } - /// - /// Enable SSL - /// - public override uint EnableSsl(uint options) - { - return _connection.EnableSsl(options); - } + public override uint EnableSsl(uint options) => _connection.EnableSsl(options); + + public override void DisableSsl() => _connection.DisableSsl(); + + public override SNIPacket RentPacket(int headerSize, int dataSize) => _connection.RentPacket(headerSize, dataSize); + + public override void ReturnPacket(SNIPacket packet) => _connection.ReturnPacket(packet); - /// - /// Disable SSL - /// - public override void DisableSsl() - { - _connection.DisableSsl(); - } #if DEBUG /// diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsQueuedPacket.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsQueuedPacket.cs index 262be701c0..0f97eb4978 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsQueuedPacket.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsQueuedPacket.cs @@ -7,10 +7,10 @@ namespace Microsoft.Data.SqlClient.SNI /// /// Mars queued packet /// - internal class SNIMarsQueuedPacket + internal sealed class SNIMarsQueuedPacket { - private SNIPacket _packet; - private SNIAsyncCallback _callback; + private readonly SNIPacket _packet; + private readonly SNIAsyncCallback _callback; /// /// Constructor @@ -23,36 +23,8 @@ public SNIMarsQueuedPacket(SNIPacket packet, SNIAsyncCallback callback) _callback = callback; } - /// - /// SNI packet - /// - public SNIPacket Packet - { - get - { - return _packet; - } - - set - { - _packet = value; - } - } + public SNIPacket Packet => _packet; - /// - /// Completion callback - /// - public SNIAsyncCallback Callback - { - get - { - return _callback; - } - - set - { - _callback = value; - } - } + public SNIAsyncCallback Callback => _callback; } } diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNINpHandle.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNINpHandle.cs index d01b22631d..ffda6c6248 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNINpHandle.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNINpHandle.cs @@ -16,7 +16,7 @@ namespace Microsoft.Data.SqlClient.SNI /// /// Named Pipe connection handle /// - internal sealed class SNINpHandle : SNIHandle + internal sealed class SNINpHandle : SNIPhysicalHandle { internal const string DefaultPipePath = @"sql\query"; // e.g. \\HOSTNAME\pipe\sql\query private const int MAX_PIPE_INSTANCES = 255; @@ -179,7 +179,7 @@ public override uint Receive(out SNIPacket packet, int timeout) packet = null; try { - packet = new SNIPacket(headerSize: 0, dataSize: _bufferSize); + packet = RentPacket(headerSize: 0, dataSize: _bufferSize); packet.ReadFromStream(_stream); if (packet.Length == 0) @@ -220,7 +220,7 @@ public override uint ReceiveAsync(ref SNIPacket packet) try { SNIPacket errorPacket; - packet = new SNIPacket(headerSize: 0, dataSize: _bufferSize); + packet = RentPacket(headerSize: 0, dataSize: _bufferSize); try { @@ -307,13 +307,13 @@ public override uint Send(SNIPacket packet) } } - public override uint SendAsync(SNIPacket packet, bool disposePacketAfterSendAsync, SNIAsyncCallback callback = null) + public override uint SendAsync(SNIPacket packet, SNIAsyncCallback callback = null) { long scopeID = SqlClientEventSource.Log.SNIScopeEnterEvent(""); try { SNIAsyncCallback cb = callback ?? _sendCallback; - packet.WriteToStreamAsync(_stream, cb, SNIProviders.NP_PROV, disposePacketAfterSendAsync); + packet.WriteToStreamAsync(_stream, cb, SNIProviders.NP_PROV); return TdsEnums.SNI_SUCCESS_IO_PENDING; } finally @@ -407,7 +407,7 @@ private uint ReportErrorAndReleasePacket(SNIPacket packet, Exception sniExceptio { if (packet != null) { - packet.Release(); + ReturnPacket(packet); } return SNICommon.ReportSNIError(SNIProviders.NP_PROV, SNICommon.InternalExceptionError, sniException); } @@ -416,7 +416,7 @@ private uint ReportErrorAndReleasePacket(SNIPacket packet, uint nativeError, uin { if (packet != null) { - packet.Release(); + ReturnPacket(packet); } return SNICommon.ReportSNIError(SNIProviders.NP_PROV, nativeError, sniError, errorMessage); } diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIPacket.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIPacket.cs index 60e3055998..0ab375b129 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIPacket.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIPacket.cs @@ -2,8 +2,11 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +// #define TRACE_HISTORY // this is used for advanced debugging when you need to trace the entire lifetime of a single packet, be very careful with it + using System; using System.Buffers; +using System.Collections.Generic; using System.Diagnostics; using System.IO; using System.Threading; @@ -23,17 +26,64 @@ internal sealed class SNIPacket // _headerOffset is not needed because it is always 0 private byte[] _data; private SNIAsyncCallback _completionCallback; - - public SNIPacket(int headerSize, int dataSize) + private readonly Action, object> _readCallback; +#if DEBUG + internal readonly int _id; // in debug mode every packet is assigned a unique id so that the entire lifetime can be tracked when debugging + /// refcount = 0 means that a packet should only exist in the pool + /// refcount = 1 means that a packet is active + /// refcount > 1 means that a packet has been reused in some way and is a serious error + internal int _refCount; + internal readonly SNIHandle _owner; // used in debug builds to check that packets are being returned to the correct pool + internal string _traceTag; // used in debug builds to assist tracing what steps the packet has been through + + [DebuggerDisplay("{Action.ToString(),nq}")] + internal struct History { - Allocate(headerSize, dataSize); + public enum Direction + { + Rent=0, + Return=1, + } + + public Direction Action; + public int RefCount; + public string Stack; } - public bool HasCompletionCallback => !(_completionCallback is null); + + internal List _history = null; /// - /// Dispose Packet data + /// uses the packet refcount in debug mode to identify if the packet is considered active + /// it is an error to use a packet which is not active in any function outside the pool implementation /// - public void Dispose() => Release(); + public bool IsActive => _refCount == 1; + + public SNIPacket(SNIHandle owner,int id) + : this() + { +#if TRACE_HISTORY + _history = new List(); +#endif + _id = id; + _owner = owner; + } + + // the finalizer is only included in debug builds and is used to ensure that all packets are correctly recycled + // it is not an error if a packet is dropped but it is undesirable so all efforts should be made to make sure we + // do not drop them for the GC to pick up + ~SNIPacket() + { + if (_data != null) + { + Debug.Fail($@"finalizer called for unreleased SNIPacket, tag: {_traceTag}"); + } + } + +#endif + public SNIPacket() + { + _readCallback = ReadFromStreamAsyncContinuation; + } /// /// Length of data left to process @@ -57,6 +107,8 @@ public SNIPacket(int headerSize, int dataSize) public int ReservedHeaderSize => _headerLength; + public bool HasCompletionCallback => !(_completionCallback is null); + /// /// Set async completion callback /// @@ -80,7 +132,7 @@ public void InvokeCompletionCallback(uint sniErrorCode) /// /// Length of packet header /// Length of byte array to be allocated - private void Allocate(int headerLength, int dataLength) + public void Allocate(int headerLength, int dataLength) { _data = ArrayPool.Shared.Rent(headerLength + dataLength); _dataCapacity = dataLength; @@ -181,6 +233,7 @@ public void Release() _dataOffset = 0; _headerLength = 0; _completionCallback = null; + IsOutOfBand = false; } /// @@ -199,37 +252,38 @@ public void ReadFromStream(Stream stream) /// Completion callback public void ReadFromStreamAsync(Stream stream, SNIAsyncCallback callback) { - bool error = false; + stream.ReadAsync(_data, 0, _dataCapacity, CancellationToken.None) + .ContinueWith( + continuationAction: _readCallback, + state: callback, + CancellationToken.None, + TaskContinuationOptions.DenyChildAttach, + TaskScheduler.Default + ); + } - stream.ReadAsync(_data, 0, _dataCapacity, CancellationToken.None).ContinueWith(t => + private void ReadFromStreamAsyncContinuation(Task t, object state) + { + SNIAsyncCallback callback = (SNIAsyncCallback)state; + bool error = false; + Exception e = t.Exception?.InnerException; + if (e != null) { - Exception e = t.Exception?.InnerException; - if (e != null) - { - SNILoadHandle.SingletonInstance.LastError = new SNIError(SNIProviders.TCP_PROV, SNICommon.InternalExceptionError, e); - error = true; - } - else - { - _dataLength = t.Result; - - if (_dataLength == 0) - { - SNILoadHandle.SingletonInstance.LastError = new SNIError(SNIProviders.TCP_PROV, 0, SNICommon.ConnTerminatedError, string.Empty); - error = true; - } - } + SNILoadHandle.SingletonInstance.LastError = new SNIError(SNIProviders.TCP_PROV, SNICommon.InternalExceptionError, e); + error = true; + } + else + { + _dataLength = t.Result; - if (error) + if (_dataLength == 0) { - Release(); + SNILoadHandle.SingletonInstance.LastError = new SNIError(SNIProviders.TCP_PROV, 0, SNICommon.ConnTerminatedError, string.Empty); + error = true; } + } - callback(this, error ? TdsEnums.SNI_ERROR : TdsEnums.SNI_SUCCESS); - }, - CancellationToken.None, - TaskContinuationOptions.DenyChildAttach, - TaskScheduler.Default); + callback(this, error ? TdsEnums.SNI_ERROR : TdsEnums.SNI_SUCCESS); } /// @@ -247,8 +301,7 @@ public void WriteToStream(Stream stream) /// Stream to write to /// SNI Asynchronous Callback /// SNI provider identifier - /// Bool flag to decide whether or not to dispose after Write Async operation - public async void WriteToStreamAsync(Stream stream, SNIAsyncCallback callback, SNIProviders provider, bool disposeAfterWriteAsync = false) + public async void WriteToStreamAsync(Stream stream, SNIAsyncCallback callback, SNIProviders provider) { uint status = TdsEnums.SNI_SUCCESS; try @@ -261,52 +314,6 @@ public async void WriteToStreamAsync(Stream stream, SNIAsyncCallback callback, S status = TdsEnums.SNI_ERROR; } callback(this, status); - - if (disposeAfterWriteAsync) - { - Dispose(); - } - } - - /// - /// Get hash code - /// - /// Hash code - public override int GetHashCode() - { - return base.GetHashCode(); - } - - /// - /// Check packet equality - /// - /// - /// true if equal - public override bool Equals(object obj) - { - SNIPacket packet = obj as SNIPacket; - - if (packet != null) - { - return Equals(packet); - } - - return false; - } - - /// - /// Check packet equality - /// - /// - /// true if equal - public bool Equals(SNIPacket packet) - { - if (packet != null) - { - return ReferenceEquals(packet, this); - } - - return false; } } } diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIPacketPool.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIPacketPool.cs new file mode 100644 index 0000000000..7f5fc1aea6 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIPacketPool.cs @@ -0,0 +1,67 @@ +// 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.Diagnostics; +using System.Threading; + +namespace Microsoft.Data.SqlClient.SNI +{ + // this is a very simple threadsafe pool derived from the aspnet/extensions default pool implementation + // https://github.com/dotnet/extensions/blob/release/3.1/src/ObjectPool/src/DefaultObjectPool.cs + internal sealed class SNIPacketPool + { + private readonly PacketWrapper[] _items; + private SNIPacket _firstItem; + + public SNIPacketPool(int maximumRetained) + { + // -1 due to _firstItem + _items = new PacketWrapper[maximumRetained - 1]; + } + + public bool TryGet(out SNIPacket packet) + { + packet = null; + SNIPacket item = _firstItem; + if (item != null && Interlocked.CompareExchange(ref _firstItem, null, item) == item) + { + // took first item + packet = item; + return true; + } + else + { + var items = _items; + for (var i = 0; i < items.Length; i++) + { + item = items[i].Element; + if (item != null && Interlocked.CompareExchange(ref items[i].Element, null, item) == item) + { + packet = item; + return true; + } + } + } + return false; + } + + public void Return(SNIPacket packet) + { + if (_firstItem != null || Interlocked.CompareExchange(ref _firstItem, packet, null) != null) + { + var items = _items; + for (var i = 0; i < items.Length && Interlocked.CompareExchange(ref items[i].Element, packet, null) != null; ++i) + { + } + } + } + + // PERF: the struct wrapper avoids array-covariance-checks from the runtime when assigning to elements of the array. + [DebuggerDisplay("{Element}")] + private struct PacketWrapper + { + public SNIPacket Element; + } + } +} diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIPhysicalHandle.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIPhysicalHandle.cs new file mode 100644 index 0000000000..65a4432f80 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIPhysicalHandle.cs @@ -0,0 +1,92 @@ +// 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.Diagnostics; +using System.Collections.Concurrent; +using System.Threading; +using System.Linq; +using System.Text; + +namespace Microsoft.Data.SqlClient.SNI +{ + internal abstract class SNIPhysicalHandle : SNIHandle + { + protected const int DefaultPoolSize = 4; +#if DEBUG + private static int s_packetId; +#endif + private SNIPacketPool _pool; + + protected SNIPhysicalHandle(int poolSize = DefaultPoolSize) + { + _pool = new SNIPacketPool(poolSize); + } + + public override SNIPacket RentPacket(int headerSize, int dataSize) + { + SNIPacket packet; + if (!_pool.TryGet(out packet)) + { +#if DEBUG + int id = Interlocked.Increment(ref s_packetId); + packet = new SNIPacket(this, id); +#else + packet = new SNIPacket(); +#endif + } +#if DEBUG + else + { + Debug.Assert(packet != null, "dequeue returned null SNIPacket"); + Debug.Assert(!packet.IsActive, "SNIPacket _refcount must be 1 or a lifetime issue has occured, trace with the #TRACE_HISTORY define"); + Debug.Assert(packet.IsInvalid, "dequeue returned valid packet"); + GC.ReRegisterForFinalize(packet); + } + if (packet._history != null) + { + packet._history.Add(new SNIPacket.History { Action = SNIPacket.History.Direction.Rent, Stack = GetStackParts(), RefCount = packet._refCount }); + } + Interlocked.Add(ref packet._refCount, 1); + Debug.Assert(packet.IsActive, "SNIPacket _refcount must be 1 or a lifetime issue has occured, trace with the #TRACE_HISTORY define"); +#endif + packet.Allocate(headerSize, dataSize); + return packet; + } + + public override void ReturnPacket(SNIPacket packet) + { + Debug.Assert(packet != null, "releasing null SNIPacket"); +#if DEBUG + Debug.Assert(packet.IsActive, "SNIPacket _refcount must be 1 or a lifetime issue has occured, trace with the #TRACE_HISTORY define"); + Debug.Assert(ReferenceEquals(packet._owner, this), "releasing SNIPacket that belongs to another physical handle"); +#endif + Debug.Assert(!packet.IsInvalid, "releasing already released SNIPacket"); + + packet.Release(); +#if DEBUG + Interlocked.Add(ref packet._refCount, -1); + packet._traceTag = null; + if (packet._history != null) + { + packet._history.Add(new SNIPacket.History { Action = SNIPacket.History.Direction.Return, Stack = GetStackParts(), RefCount = packet._refCount }); + } + GC.SuppressFinalize(packet); +#endif + _pool.Return(packet); + } + +#if DEBUG + private string GetStackParts() + { + return string.Join(Environment.NewLine, + Environment.StackTrace + .Split(new string[] { Environment.NewLine },StringSplitOptions.None) + .Skip(3) // trims off the common parts at the top of the stack so you can see what the actual caller was + .Take(7) // trims off most of the bottom of the stack because when running under xunit there's a lot of spam + ); + } +#endif + } +} diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIProxy.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIProxy.cs index 1d8b13a6b2..dd26939d29 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIProxy.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIProxy.cs @@ -240,11 +240,11 @@ public uint WritePacket(SNIHandle handle, SNIPacket packet, bool sync) if (sync) { result = handle.Send(packet); - packet.Release(); + handle.ReturnPacket(packet); } else { - result = handle.SendAsync(packet, true); + result = handle.SendAsync(packet); } return result; @@ -458,15 +458,6 @@ public void PacketSetData(SNIPacket packet, byte[] data, int length) packet.AppendData(data, length); } - /// - /// Release packet - /// - /// SNI packet - public void PacketRelease(SNIPacket packet) - { - packet.Release(); - } - /// /// Check SNI handle connection /// diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNITcpHandle.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNITcpHandle.cs index ac4e819d1d..158c13949a 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNITcpHandle.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNITcpHandle.cs @@ -20,7 +20,7 @@ namespace Microsoft.Data.SqlClient.SNI /// /// TCP connection handle /// - internal sealed class SNITCPHandle : SNIHandle + internal sealed class SNITCPHandle : SNIPhysicalHandle { private readonly string _targetServer; private readonly object _callbackObject; @@ -513,7 +513,7 @@ public override uint Receive(out SNIPacket packet, int timeoutInMilliseconds) return TdsEnums.SNI_WAIT_TIMEOUT; } - packet = new SNIPacket(headerSize: 0, dataSize: _bufferSize); + packet = RentPacket(headerSize: 0, dataSize: _bufferSize); packet.ReadFromStream(_stream); if (packet.Length == 0) @@ -572,15 +572,14 @@ public override void SetAsyncCallbacks(SNIAsyncCallback receiveCallback, SNIAsyn /// Send a packet asynchronously /// /// SNI packet - /// /// Completion callback /// SNI error code - public override uint SendAsync(SNIPacket packet, bool disposePacketAfterSendAsync, SNIAsyncCallback callback = null) + public override uint SendAsync(SNIPacket packet, SNIAsyncCallback callback = null) { SNIAsyncCallback cb = callback ?? _sendCallback; lock (this) { - packet.WriteToStreamAsync(_stream, cb, SNIProviders.TCP_PROV, disposePacketAfterSendAsync); + packet.WriteToStreamAsync(_stream, cb, SNIProviders.TCP_PROV); } return TdsEnums.SNI_SUCCESS_IO_PENDING; } @@ -593,7 +592,7 @@ public override uint SendAsync(SNIPacket packet, bool disposePacketAfterSendAsyn public override uint ReceiveAsync(ref SNIPacket packet) { SNIPacket errorPacket; - packet = new SNIPacket(headerSize: 0, dataSize: _bufferSize); + packet = RentPacket(headerSize: 0, dataSize: _bufferSize); try { @@ -659,7 +658,7 @@ private uint ReportErrorAndReleasePacket(SNIPacket packet, Exception sniExceptio { if (packet != null) { - packet.Release(); + ReturnPacket(packet); } return ReportTcpSNIError(sniException); } @@ -668,7 +667,7 @@ private uint ReportErrorAndReleasePacket(SNIPacket packet, uint nativeError, uin { if (packet != null) { - packet.Release(); + ReturnPacket(packet); } return ReportTcpSNIError(nativeError, sniError, errorMessage); } diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs index 3efac3d278..46ae4c4dfb 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs @@ -59,15 +59,21 @@ internal override void CreatePhysicalSNIHandle(string serverName, bool ignoreSni else if (async) { // Create call backs and allocate to the session handle - SNIAsyncCallback ReceiveAsyncCallbackDispatcher = new SNIAsyncCallback(ReadAsyncCallback); - SNIAsyncCallback SendAsyncCallbackDispatcher = new SNIAsyncCallback(WriteAsyncCallback); - _sessionHandle.SetAsyncCallbacks(ReceiveAsyncCallbackDispatcher, SendAsyncCallbackDispatcher); + _sessionHandle.SetAsyncCallbacks(ReadAsyncCallback, WriteAsyncCallback); } } - internal void ReadAsyncCallback(SNIPacket packet, uint error) => ReadAsyncCallback(IntPtr.Zero, PacketHandle.FromManagedPacket(packet), error); + internal void ReadAsyncCallback(SNIPacket packet, uint error) + { + ReadAsyncCallback(IntPtr.Zero, PacketHandle.FromManagedPacket(packet), error); + _sessionHandle.ReturnPacket(packet); + } - internal void WriteAsyncCallback(SNIPacket packet, uint sniError) => WriteAsyncCallback(IntPtr.Zero, PacketHandle.FromManagedPacket(packet), sniError); + internal void WriteAsyncCallback(SNIPacket packet, uint sniError) + { + WriteAsyncCallback(IntPtr.Zero, PacketHandle.FromManagedPacket(packet), sniError); + _sessionHandle.ReturnPacket(packet); + } protected override void RemovePacketFromPendingList(PacketHandle packet) { @@ -109,8 +115,7 @@ internal override PacketHandle ReadSyncOverAsync(int timeoutRemaining, out uint { throw ADP.ClosedConnectionError(); } - SNIPacket packet = null; - error = SNIProxy.Singleton.ReadSyncOverAsync(handle, out packet, timeoutRemaining); + error = SNIProxy.Singleton.ReadSyncOverAsync(handle, out SNIPacket packet, timeoutRemaining); return PacketHandle.FromManagedPacket(packet); } @@ -118,7 +123,15 @@ internal override PacketHandle ReadSyncOverAsync(int timeoutRemaining, out uint internal override bool IsPacketEmpty(PacketHandle packet) => packet.ManagedPacket == null; - internal override void ReleasePacket(PacketHandle syncReadPacket) => syncReadPacket.ManagedPacket?.Release(); + internal override void ReleasePacket(PacketHandle syncReadPacket) + { + SNIPacket packet = syncReadPacket.ManagedPacket; + if (packet != null) + { + SNIHandle handle = Handle; + handle.ReturnPacket(packet); + } + } internal override uint CheckConnection() { @@ -135,6 +148,9 @@ internal override PacketHandle ReadAsync(SessionHandle handle, out uint error) internal override PacketHandle CreateAndSetAttentionPacket() { PacketHandle packetHandle = GetResetWritePacket(TdsEnums.HEADER_LEN); +#if DEBUG + Debug.Assert(packetHandle.ManagedPacket.IsActive, "rental packet is not active a serious pooling error may have occured"); +#endif SetPacketData(packetHandle, SQL.AttentionHeader, TdsEnums.HEADER_LEN); packetHandle.ManagedPacket.IsOutOfBand = true; return packetHandle; @@ -158,8 +174,12 @@ internal override bool IsValidPacket(PacketHandle packet) internal override PacketHandle GetResetWritePacket(int dataSize) { - var packet = new SNIPacket(headerSize: _sessionHandle.ReserveHeaderSize, dataSize: dataSize); - Debug.Assert(packet.ReservedHeaderSize == _sessionHandle.ReserveHeaderSize, "failed to reserve header"); + SNIHandle handle = Handle; + SNIPacket packet = handle.RentPacket(headerSize: handle.ReserveHeaderSize, dataSize: dataSize); +#if DEBUG + Debug.Assert(packet.IsActive, "packet is not active, a serious pooling error may have occured"); +#endif + Debug.Assert(packet.ReservedHeaderSize == handle.ReserveHeaderSize, "failed to reserve header"); return PacketHandle.FromManagedPacket(packet); }