From 1055b51947b1dabbd1f1e2cca0beefdedda77153 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Tue, 11 Dec 2018 11:28:14 +0000 Subject: [PATCH 1/8] add a pre-boxed invalid prepared handle constant to remove repeated boxing of -1 --- .../src/System/Data/SqlClient/SqlCommand.cs | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlCommand.cs b/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlCommand.cs index 53abc5286adb..abaff997b923 100644 --- a/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlCommand.cs +++ b/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlCommand.cs @@ -51,9 +51,9 @@ private enum EXECTYPE // The OnReturnValue function will test this flag to determine whether the returned value is a _prepareHandle or something else. // // _prepareHandle - the handle of a prepared command. Apparently there can be multiple prepared commands at a time - a feature that we do not support yet. - + private static readonly object s_cachedInvalidPrepareHandle = (object)-1; private bool _inPrepare = false; - private int _prepareHandle = -1; + private object _prepareHandle = s_cachedInvalidPrepareHandle; // this is an int which is used in the object typed SqlParameter.Value field, avoid repeated boxing by storing in a box private bool _hiddenPrepare = false; private int _preparedConnectionCloseCount = -1; private int _preparedConnectionReconnectCount = -1; @@ -83,7 +83,7 @@ internal bool InPrepare } // Cached info for async executions - private class CachedAsyncState + private sealed class CachedAsyncState { private int _cachedAsyncCloseCount = -1; // value of the connection's CloseCount property when the asyncResult was set; tracks when connections are closed after an async operation private TaskCompletionSource _cachedAsyncResult = null; @@ -261,7 +261,7 @@ private SqlCommand(SqlCommand from) : this() // Don't allow the connection to be changed while in an async operation. if (_activeConnection != value && _activeConnection != null) { // If new value... - if (cachedAsyncState.PendingAsyncOperation) + if (_cachedAsyncState != null && _cachedAsyncState.PendingAsyncOperation) { // If in pending async state, throw. throw SQL.CannotModifyPropertyAsyncOperationInProgress(); } @@ -292,7 +292,7 @@ private SqlCommand(SqlCommand from) : this() finally { // clean prepare status (even successful Unprepare does not do that) - _prepareHandle = -1; + _prepareHandle = s_cachedInvalidPrepareHandle; _execType = EXECTYPE.UNPREPARED; } } @@ -672,7 +672,7 @@ internal void Unprepare() if ((_activeConnection.CloseCount != _preparedConnectionCloseCount) || (_activeConnection.ReconnectCount != _preparedConnectionReconnectCount)) { // reset our handle - _prepareHandle = -1; + _prepareHandle = s_cachedInvalidPrepareHandle; } _cachedMetaData = null; @@ -2568,7 +2568,7 @@ private SqlDataReader RunExecuteReaderTds(CommandBehavior cmdBehavior, RunBehavi if (_execType == EXECTYPE.PREPARED) { - Debug.Assert(this.IsPrepared && (_prepareHandle != -1), "invalid attempt to call sp_execute without a handle!"); + Debug.Assert(this.IsPrepared && ((int)_prepareHandle != -1), "invalid attempt to call sp_execute without a handle!"); rpc = BuildExecute(inSchema); } else if (_execType == EXECTYPE.PREPAREPENDING) @@ -2863,16 +2863,16 @@ private void ValidateCommand(bool async, [CallerMemberName] string method = "") private void ValidateAsyncCommand() { - if (cachedAsyncState.PendingAsyncOperation) + if (_cachedAsyncState != null && _cachedAsyncState.PendingAsyncOperation) { // Enforce only one pending async execute at a time. - if (cachedAsyncState.IsActiveConnectionValid(_activeConnection)) + if (_cachedAsyncState.IsActiveConnectionValid(_activeConnection)) { throw SQL.PendingBeginXXXExists(); } else { _stateObj = null; // Session was re-claimed by session pool upon connection close. - cachedAsyncState.ResetAsyncState(); + _cachedAsyncState.ResetAsyncState(); } } } @@ -3371,7 +3371,7 @@ private void BuildRPC(bool inSchema, SqlParameterCollection parameters, ref _Sql private _SqlRPC BuildExecute(bool inSchema) { - Debug.Assert(_prepareHandle != -1, "Invalid call to sp_execute without a valid handle!"); + Debug.Assert((int)_prepareHandle != -1, "Invalid call to sp_execute without a valid handle!"); int j = 1; int count = CountSendableParameters(_parameters); @@ -3401,7 +3401,7 @@ private _SqlRPC BuildExecute(bool inSchema) private void BuildExecuteSql(CommandBehavior behavior, string commandText, SqlParameterCollection parameters, ref _SqlRPC rpc) { - Debug.Assert(_prepareHandle == -1, "This command has an existing handle, use sp_execute!"); + Debug.Assert((int)_prepareHandle == -1, "This command has an existing handle, use sp_execute!"); Debug.Assert(CommandType.Text == this.CommandType, "invalid use of sp_executesql for stored proc invocation!"); int j; SqlParameter sqlParam; From dfcc2466b83641024ebae369d3d65dd4555dad10 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Tue, 11 Dec 2018 11:29:05 +0000 Subject: [PATCH 2/8] change Task.Run to Task.Factory.StartNew(state) avoiding method scope closure allocation unless needed --- .../src/System/Data/SqlClient/SqlConnection.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlConnection.cs b/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlConnection.cs index 28676a079b91..d23e602887ae 100644 --- a/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlConnection.cs +++ b/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlConnection.cs @@ -911,7 +911,8 @@ internal Task ValidateAndReconnect(Action beforeDisconnect, int timeout) catch (SqlException) { } - runningReconnect = Task.Run(() => ReconnectAsync(timeout)); + // use Task.Factory.StartNew with state overload instead of Task.Run to avoid anonymous closure context capture in method scope and avoid the unneeded allocation + runningReconnect = Task.Factory.StartNew(state => ReconnectAsync((int)state), timeout,CancellationToken.None, TaskCreationOptions.DenyChildAttach,TaskScheduler.Default); // if current reconnect is not null, somebody already started reconnection task - some kind of race condition Debug.Assert(_currentReconnectionTask == null, "Duplicate reconnection tasks detected"); _currentReconnectionTask = runningReconnect; From e843af023a6fe02d9df5d6d53bb647c4dd02495f Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Tue, 11 Dec 2018 11:31:48 +0000 Subject: [PATCH 3/8] move several continuation functions to separate functions to avoid unnecasary closure scope allocations --- .../src/System/Data/SqlClient/TdsParser.cs | 92 +++++++++++++------ 1 file changed, 65 insertions(+), 27 deletions(-) diff --git a/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs b/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs index e7e35d351c0e..20f7d5601844 100644 --- a/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs +++ b/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs @@ -2135,7 +2135,7 @@ internal bool TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataRead { // Dev11 #344723: SqlClient stress hang System_Data!Tcp::ReadSync via a call to SqlDataReader::Close // Spin until SendAttention has cleared _attentionSending, this prevents a race condition between receiving the attention ACK and setting _attentionSent - SpinWait.SpinUntil(() => !stateObj._attentionSending); + TryRunSetupSpinWaitContinuation(stateObj); Debug.Assert(stateObj._attentionSent, "Attention ACK has been received without attention sent"); if (stateObj._attentionSent) @@ -2159,6 +2159,12 @@ internal bool TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataRead return true; } + // This is in its own method to avoid always allocating the lambda in TryRun + private static void TryRunSetupSpinWaitContinuation(TdsParserStateObject stateObj) + { + SpinWait.SpinUntil(() => !stateObj._attentionSending); + } + private bool TryProcessEnvChange(int tokenLength, TdsParserStateObject stateObj, out SqlEnvChange[] sqlEnvChange) { // There could be multiple environment change messages following this token. @@ -6972,29 +6978,36 @@ internal Task TdsExecuteSQLBatch(string text, int timeout, SqlNotificationReques // Need to wait for flush - continuation will unlock the connection bool taskReleaseConnectionLock = releaseConnectionLock; releaseConnectionLock = false; - return executeTask.ContinueWith(t => - { - Debug.Assert(!t.IsCanceled, "Task should not be canceled"); - try + return executeTask.ContinueWith( + (task, state) => { - if (t.IsFaulted) - { - FailureCleanup(stateObj, t.Exception.InnerException); - throw t.Exception.InnerException; - } - else + Debug.Assert(!task.IsCanceled, "Task should not be canceled"); + var parameters = (Tuple)state; + TdsParserStateObject tdsParserStateObject = parameters.Item1; + try { - stateObj.SniContext = SniContext.Snix_Read; + if (task.IsFaulted) + { + FailureCleanup(tdsParserStateObject, task.Exception.InnerException); + throw task.Exception.InnerException; + } + else + { + tdsParserStateObject.SniContext = SniContext.Snix_Read; + } } - } - finally - { - if (taskReleaseConnectionLock) + finally { - _connHandler._parserLock.Release(); + if (parameters.Item2) + { + parameters.Item3._parserLock.Release(); + } } - } - }, TaskScheduler.Default); + + }, + Tuple.Create(stateObj, taskReleaseConnectionLock, taskReleaseConnectionLock ? _connHandler : null), + TaskScheduler.Default + ); } // Finished sync @@ -7494,11 +7507,7 @@ internal Task TdsExecuteRPC(_SqlRPC[] rpcArray, int timeout, bool inSchema, SqlN task = completion.Task; } - AsyncHelper.ContinueTask(writeParamTask, completion, - () => TdsExecuteRPC(rpcArray, timeout, inSchema, notificationRequest, stateObj, isCommandProc, sync, completion, - startRpc: ii, startParam: i + 1), - connectionToDoom: _connHandler, - onFailure: exc => TdsExecuteRPC_OnFailure(exc, stateObj)); + TDSExecuteRPCParameterSetupWriteCompletion(rpcArray, timeout, inSchema, notificationRequest, stateObj, isCommandProc, sync, completion, ii, i, writeParamTask); // Take care of releasing the locks if (releaseConnectionLock) @@ -7547,10 +7556,9 @@ internal Task TdsExecuteRPC(_SqlRPC[] rpcArray, int timeout, bool inSchema, SqlN task = completion.Task; } - bool taskReleaseConnectionLock = releaseConnectionLock; - execFlushTask.ContinueWith(tsk => ExecuteFlushTaskCallback(tsk, stateObj, completion, taskReleaseConnectionLock), TaskScheduler.Default); + TDSExecuteRPCParameterSetupFlushCompletion(stateObj, completion, execFlushTask, releaseConnectionLock); - // ExecuteFlushTaskCallback will take care of the locks for us + // TDSExecuteRPCParameterSetupFlushCompletion calling ExecuteFlushTaskCallback will take care of the locks for us releaseConnectionLock = false; return task; @@ -7597,6 +7605,36 @@ internal Task TdsExecuteRPC(_SqlRPC[] rpcArray, int timeout, bool inSchema, SqlN } } + + // This is in its own method to avoid always allocating the lambda in TDSExecuteRPCParameter + private void TDSExecuteRPCParameterSetupWriteCompletion(_SqlRPC[] rpcArray, int timeout, bool inSchema, SqlNotificationRequest notificationRequest, TdsParserStateObject stateObj, bool isCommandProc, bool sync, TaskCompletionSource completion, int ii, int i, Task writeParamTask) + { + AsyncHelper.ContinueTask( + writeParamTask, + completion, + () => TdsExecuteRPC( + rpcArray, + timeout, + inSchema, + notificationRequest, + stateObj, + isCommandProc, + sync, + completion, + startRpc: ii, + startParam: i + 1 + ), + connectionToDoom: _connHandler, + onFailure: exc => TdsExecuteRPC_OnFailure(exc, stateObj) + ); + } + + // This is in its own method to avoid always allocating the lambda in TDSExecuteRPCParameter + private void TDSExecuteRPCParameterSetupFlushCompletion(TdsParserStateObject stateObj, TaskCompletionSource completion, Task execFlushTask, bool taskReleaseConnectionLock) + { + execFlushTask.ContinueWith(tsk => ExecuteFlushTaskCallback(tsk, stateObj, completion, taskReleaseConnectionLock), TaskScheduler.Default); + } + private void FinalizeExecuteRPC(TdsParserStateObject stateObj) { stateObj.SniContext = SniContext.Snix_Read; From 449bf1ee5d9fd739140c1ae30d5072f88d3c3aaa Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Tue, 11 Dec 2018 11:32:32 +0000 Subject: [PATCH 4/8] remove unused readPacket definition, allocation and cleanup --- .../Data/SqlClient/TdsParserStateObject.cs | 36 ++++++------------- 1 file changed, 11 insertions(+), 25 deletions(-) diff --git a/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObject.cs b/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObject.cs index dcc7eae1744e..a55e4b59ff10 100644 --- a/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObject.cs +++ b/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObject.cs @@ -2418,37 +2418,23 @@ internal bool IsConnectionAlive(bool throwOnException) else { uint error; + SniContext = SniContext.Snix_Connect; - object readPacket = EmptyReadPacket; - - try + error = CheckConnection(); + if ((error != TdsEnums.SNI_SUCCESS) && (error != TdsEnums.SNI_WAIT_TIMEOUT)) { - SniContext = SniContext.Snix_Connect; - - error = CheckConnection(); - - if ((error != TdsEnums.SNI_SUCCESS) && (error != TdsEnums.SNI_WAIT_TIMEOUT)) - { - // Connection is dead - isAlive = false; - if (throwOnException) - { - // Get the error from SNI so that we can throw the correct exception - AddError(_parser.ProcessSNIError(this)); - ThrowExceptionAndWarning(); - } - } - else + // Connection is dead + isAlive = false; + if (throwOnException) { - _lastSuccessfulIOTimer._value = DateTime.UtcNow.Ticks; + // Get the error from SNI so that we can throw the correct exception + AddError(_parser.ProcessSNIError(this)); + ThrowExceptionAndWarning(); } } - finally + else { - if (!IsPacketEmpty(readPacket)) - { - ReleasePacket(readPacket); - } + _lastSuccessfulIOTimer._value = DateTime.UtcNow.Ticks; } } } From d112b7cf2f774e7a415564d7de707cced1c4d97f Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Tue, 11 Dec 2018 11:33:18 +0000 Subject: [PATCH 5/8] add pre-boxed empty read packet variable to avoid boxing an int on every EmptyReadPacket property access --- .../src/System/Data/SqlClient/TdsParserStateObjectNative.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectNative.cs b/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectNative.cs index c1fa34bd9c70..e830fc074de6 100644 --- a/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectNative.cs +++ b/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectNative.cs @@ -12,6 +12,8 @@ namespace System.Data.SqlClient { internal class TdsParserStateObjectNative : TdsParserStateObject { + private static readonly object s_cachedEmptyReadPacketObjectPointer = (object)IntPtr.Zero; + private SNIHandle _sessionHandle = null; // the SNI handle we're to work on private SNIPacket _sniPacket = null; // Will have to re-vamp this for MARS @@ -35,7 +37,7 @@ internal TdsParserStateObjectNative(TdsParser parser, TdsParserStateObject physi internal override object SessionHandle => _sessionHandle; - protected override object EmptyReadPacket => IntPtr.Zero; + protected override object EmptyReadPacket => s_cachedEmptyReadPacketObjectPointer; protected override void CreateSessionHandle(TdsParserStateObject physicalConnection, bool async) { From c442521a4041b55accfef8fb8934bbc36aeea025 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Tue, 11 Dec 2018 11:43:40 +0000 Subject: [PATCH 6/8] refine state to remove this closure, minor formatting --- .../src/System/Data/SqlClient/TdsParser.cs | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs b/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs index 20f7d5601844..6a01db834b80 100644 --- a/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs +++ b/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs @@ -136,7 +136,7 @@ internal sealed partial class TdsParser internal TdsParser(bool MARS, bool fAsynchronous) { _fMARS = MARS; // may change during Connect to pre Yukon servers - + _physicalStateObj = TdsParserStateObjectFactory.Singleton.CreateTdsParserStateObject(this); } @@ -388,7 +388,7 @@ internal void Connect(ServerInfo serverInfo, SqlInternalConnectionTds connHandle Debug.Assert(retCode == TdsEnums.SNI_SUCCESS, "Unexpected failure state upon calling SniGetConnectionId"); SendPreLoginHandshake(instanceName, encrypt); - status = ConsumePreLoginHandshake(encrypt, trustServerCert, integratedSecurity, out marsCapable, out _connHandler._fedAuthRequired); + status = ConsumePreLoginHandshake(encrypt, trustServerCert, integratedSecurity, out marsCapable, out _connHandler._fedAuthRequired); // Don't need to check for Sphinx failure, since we've already consumed // one pre-login packet and know we are connecting to Shiloh. @@ -424,7 +424,7 @@ internal void RemoveEncryption() // create a new packet encryption changes the internal packet size _physicalStateObj.ClearAllWritePackets(); - + } internal void EnableMars() @@ -434,7 +434,7 @@ internal void EnableMars() // Cache physical stateObj and connection. _pMarsPhysicalConObj = _physicalStateObj; - if(TdsParserStateObjectFactory.UseManagedSNI) _pMarsPhysicalConObj.IncrementPendingCallbacks(); + if (TdsParserStateObjectFactory.UseManagedSNI) _pMarsPhysicalConObj.IncrementPendingCallbacks(); uint info = 0; uint error = _pMarsPhysicalConObj.EnableMars(ref info); @@ -658,7 +658,7 @@ private void SendPreLoginHandshake(byte[] instanceName, bool encrypt) _physicalStateObj.WritePacket(TdsEnums.HARDFLUSH); } - private PreLoginHandshakeStatus ConsumePreLoginHandshake(bool encrypt, bool trustServerCert, bool integratedSecurity, out bool marsCapable, out bool fedAuthRequired ) + private PreLoginHandshakeStatus ConsumePreLoginHandshake(bool encrypt, bool trustServerCert, bool integratedSecurity, out bool marsCapable, out bool fedAuthRequired) { marsCapable = _fMARS; // Assign default value fedAuthRequired = false; @@ -802,7 +802,7 @@ private PreLoginHandshakeStatus ConsumePreLoginHandshake(bool encrypt, bool trus } WaitForSSLHandShakeToComplete(ref error); - + // create a new packet encryption changes the internal packet size _physicalStateObj.ClearAllWritePackets(); } @@ -1149,7 +1149,7 @@ internal SqlError ProcessSNIError(TdsParserStateObject stateObj) Debug.Assert(SniContext.Undefined != stateObj.DebugOnlyCopyOfSniContext || ((_fMARS) && ((_state == TdsParserState.Closed) || (_state == TdsParserState.Broken))), "SniContext must not be None"); #endif SNIErrorDetails details = GetSniErrorDetails(); - + if (details.sniErrorNumber != 0) { // handle special SNI error codes that are converted into exception which is not a SqlException. @@ -2287,7 +2287,7 @@ private bool TryProcessEnvChange(int tokenLength, TdsParserStateObject stateObj, { return false; } - + // Give the parser the new collation values in case parameters don't specify one _defaultCollation = env.newCollation; @@ -6223,7 +6223,7 @@ internal void TdsLogin(SqlLogin rec, TdsEnums.FeatureExtension requestedFeatures _physicalStateObj.SniContext = SniContext.Snix_LoginSspi; SSPIData(null, 0, ref outSSPIBuff, ref outSSPILength); - + if (outSSPILength > int.MaxValue) { throw SQL.InvalidSSPIPacketSize(); // SqlBu 332503 @@ -6547,7 +6547,7 @@ private void SNISSPIData(byte[] receivedBuff, uint receivedLength, ref byte[] se } } else - { + { if (receivedBuff == null) { // if we do not have SSPI data coming from server, send over 0's for pointer and length @@ -6982,13 +6982,13 @@ internal Task TdsExecuteSQLBatch(string text, int timeout, SqlNotificationReques (task, state) => { Debug.Assert(!task.IsCanceled, "Task should not be canceled"); - var parameters = (Tuple)state; - TdsParserStateObject tdsParserStateObject = parameters.Item1; + var parameters = (Tuple)state; + TdsParserStateObject tdsParserStateObject = parameters.Item2; try { if (task.IsFaulted) { - FailureCleanup(tdsParserStateObject, task.Exception.InnerException); + parameters.Item1.FailureCleanup(tdsParserStateObject, task.Exception.InnerException); throw task.Exception.InnerException; } else @@ -6998,14 +6998,13 @@ internal Task TdsExecuteSQLBatch(string text, int timeout, SqlNotificationReques } finally { - if (parameters.Item2) + if (parameters.Item3) { - parameters.Item3._parserLock.Release(); + parameters.Item4._parserLock.Release(); } } - }, - Tuple.Create(stateObj, taskReleaseConnectionLock, taskReleaseConnectionLock ? _connHandler : null), + Tuple.Create(this, stateObj, taskReleaseConnectionLock, taskReleaseConnectionLock ? _connHandler : null), TaskScheduler.Default ); } From 08eaf0390e26192b9c71b2894165415dfdce607c Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Tue, 11 Dec 2018 21:09:23 +0000 Subject: [PATCH 7/8] added AsyncHelper.*WithState methods to avoid closure manually changed multiple callsites to use new methods changed multiple callsites to extract closure where state method is not possible --- .../src/System/Data/SqlClient/SqlCommand.cs | 151 +++++++++++------- .../System/Data/SqlClient/SqlConnection.cs | 2 +- .../src/System/Data/SqlClient/SqlUtil.cs | 74 +++++++++ 3 files changed, 170 insertions(+), 57 deletions(-) diff --git a/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlCommand.cs b/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlCommand.cs index abaff997b923..1ecd3f49039a 100644 --- a/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlCommand.cs +++ b/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlCommand.cs @@ -918,7 +918,13 @@ public IAsyncResult BeginExecuteNonQuery(AsyncCallback callback, object stateObj cachedAsyncState.SetActiveConnectionAndResult(completion, nameof(EndExecuteNonQuery), _activeConnection); if (execNQ != null) { - AsyncHelper.ContinueTask(execNQ, completion, () => BeginExecuteNonQueryInternalReadStage(completion)); + AsyncHelper.ContinueTaskWithState(execNQ, completion, + state: Tuple.Create(this, completion), + onSuccess: state => { + var parameters = (Tuple>)state; + parameters.Item1.BeginExecuteNonQueryInternalReadStage(parameters.Item2); + } + ); } else { @@ -1170,7 +1176,10 @@ private Task InternalExecuteNonQuery(TaskCompletionSource completion, bo { if (task != null) { - task = AsyncHelper.CreateContinuationTask(task, () => reader.Close()); + task = AsyncHelper.CreateContinuationTaskWithState(task, + state: reader, + onSuccess: state => ((SqlDataReader)state).Close() + ); } else { @@ -1265,7 +1274,13 @@ public IAsyncResult BeginExecuteXmlReader(AsyncCallback callback, object stateOb cachedAsyncState.SetActiveConnectionAndResult(completion, nameof(EndExecuteXmlReader), _activeConnection); if (writeTask != null) { - AsyncHelper.ContinueTask(writeTask, completion, () => BeginExecuteXmlReaderInternalReadStage(completion)); + AsyncHelper.ContinueTaskWithState(writeTask, completion, + state: Tuple.Create(this, completion), + onSuccess: state => { + var parameters = (Tuple>)state; + parameters.Item1.BeginExecuteXmlReaderInternalReadStage(parameters.Item2); + } + ); } else { @@ -1528,7 +1543,13 @@ internal IAsyncResult BeginExecuteReader(CommandBehavior behavior, AsyncCallback cachedAsyncState.SetActiveConnectionAndResult(completion, nameof(EndExecuteReader), _activeConnection); if (writeTask != null) { - AsyncHelper.ContinueTask(writeTask, completion, () => BeginExecuteReaderInternalReadStage(completion)); + AsyncHelper.ContinueTaskWithState(writeTask, completion, + state: Tuple.Create(this, completion), + onSuccess: state => { + var parameters = (Tuple>)state; + parameters.Item1.BeginExecuteReaderInternalReadStage(parameters.Item2); + } + ); } else { @@ -2328,27 +2349,7 @@ private Task RunExecuteNonQueryTds(string methodName, bool async, int timeout, b TaskCompletionSource completion = new TaskCompletionSource(); _activeConnection.RegisterWaitingForReconnect(completion.Task); _reconnectionCompletionSource = completion; - CancellationTokenSource timeoutCTS = new CancellationTokenSource(); - AsyncHelper.SetTimeoutException(completion, timeout, SQL.CR_ReconnectTimeout, timeoutCTS.Token); - AsyncHelper.ContinueTask(reconnectTask, completion, - () => - { - if (completion.Task.IsCompleted) - { - return; - } - Interlocked.CompareExchange(ref _reconnectionCompletionSource, null, completion); - timeoutCTS.Cancel(); - Task subTask = RunExecuteNonQueryTds(methodName, async, TdsParserStaticMethods.GetRemainingTimeout(timeout, reconnectionStart), asyncWrite); - if (subTask == null) - { - completion.SetResult(null); - } - else - { - AsyncHelper.ContinueTask(subTask, completion, () => completion.SetResult(null)); - } - }, connectionToAbort: _activeConnection); + RunExecuteNonQueryTdsSetupReconnnectContinuation(methodName, async, timeout, asyncWrite, reconnectTask, reconnectionStart, completion); return completion.Task; } else @@ -2401,6 +2402,31 @@ private Task RunExecuteNonQueryTds(string methodName, bool async, int timeout, b return null; } + // This is in its own method to avoid always allocating the lambda in RunExecuteNonQueryTds, cannot use ContinueTaskWithState because of MarshalByRef and the CompareExchange + private void RunExecuteNonQueryTdsSetupReconnnectContinuation(string methodName, bool async, int timeout, bool asyncWrite, Task reconnectTask, long reconnectionStart, TaskCompletionSource completion) + { + CancellationTokenSource timeoutCTS = new CancellationTokenSource(); + AsyncHelper.SetTimeoutException(completion, timeout, SQL.CR_ReconnectTimeout, timeoutCTS.Token); + AsyncHelper.ContinueTask(reconnectTask, completion, + () => + { + if (completion.Task.IsCompleted) + { + return; + } + Interlocked.CompareExchange(ref _reconnectionCompletionSource, null, completion); + timeoutCTS.Cancel(); + Task subTask = RunExecuteNonQueryTds(methodName, async, TdsParserStaticMethods.GetRemainingTimeout(timeout, reconnectionStart), asyncWrite); + if (subTask == null) + { + completion.SetResult(null); + } + else + { + AsyncHelper.ContinueTask(subTask, completion, () => completion.SetResult(null)); + } + }, connectionToAbort: _activeConnection); + } internal SqlDataReader RunExecuteReader(CommandBehavior cmdBehavior, RunBehavior runBehavior, bool returnStream, [CallerMemberName] string method = "") { @@ -2470,28 +2496,7 @@ private SqlDataReader RunExecuteReaderTds(CommandBehavior cmdBehavior, RunBehavi TaskCompletionSource completion = new TaskCompletionSource(); _activeConnection.RegisterWaitingForReconnect(completion.Task); _reconnectionCompletionSource = completion; - CancellationTokenSource timeoutCTS = new CancellationTokenSource(); - AsyncHelper.SetTimeoutException(completion, timeout, SQL.CR_ReconnectTimeout, timeoutCTS.Token); - AsyncHelper.ContinueTask(reconnectTask, completion, - () => - { - if (completion.Task.IsCompleted) - { - return; - } - Interlocked.CompareExchange(ref _reconnectionCompletionSource, null, completion); - timeoutCTS.Cancel(); - Task subTask; - RunExecuteReaderTds(cmdBehavior, runBehavior, returnStream, async, TdsParserStaticMethods.GetRemainingTimeout(timeout, reconnectionStart), out subTask, asyncWrite, ds); - if (subTask == null) - { - completion.SetResult(null); - } - else - { - AsyncHelper.ContinueTask(subTask, completion, () => completion.SetResult(null)); - } - }, connectionToAbort: _activeConnection); + RunExecuteReaderTdsSetupReconnectContinuation(cmdBehavior, runBehavior, returnStream, async, timeout, asyncWrite, ds, reconnectTask, reconnectionStart, completion); task = completion.Task; return ds; } @@ -2627,15 +2632,7 @@ private SqlDataReader RunExecuteReaderTds(CommandBehavior cmdBehavior, RunBehavi decrementAsyncCountOnFailure = false; if (writeTask != null) { - task = AsyncHelper.CreateContinuationTask(writeTask, () => - { - _activeConnection.GetOpenTdsConnection(); // it will throw if connection is closed - cachedAsyncState.SetAsyncReaderState(ds, runBehavior, optionSettings); - }, - onFailure: (exc) => - { - _activeConnection.GetOpenTdsConnection().DecrementAsyncCount(); - }); + task = RunExecuteReaderTdsSetupContinuation(runBehavior, ds, optionSettings, writeTask); } else { @@ -2674,6 +2671,48 @@ private SqlDataReader RunExecuteReaderTds(CommandBehavior cmdBehavior, RunBehavi return ds; } + // This is in its own method to avoid always allocating the lambda in RunExecuteReaderTds + private Task RunExecuteReaderTdsSetupContinuation(RunBehavior runBehavior, SqlDataReader ds, string optionSettings, Task writeTask) + { + Task task = AsyncHelper.CreateContinuationTask(writeTask, () => + { + _activeConnection.GetOpenTdsConnection(); // it will throw if connection is closed + cachedAsyncState.SetAsyncReaderState(ds, runBehavior, optionSettings); + }, + onFailure: (exc) => + { + _activeConnection.GetOpenTdsConnection().DecrementAsyncCount(); + }); + return task; + } + + // This is in its own method to avoid always allocating the lambda in RunExecuteReaderTds + private void RunExecuteReaderTdsSetupReconnectContinuation(CommandBehavior cmdBehavior, RunBehavior runBehavior, bool returnStream, bool async, int timeout, bool asyncWrite, SqlDataReader ds, Task reconnectTask, long reconnectionStart, TaskCompletionSource completion) + { + CancellationTokenSource timeoutCTS = new CancellationTokenSource(); + AsyncHelper.SetTimeoutException(completion, timeout, SQL.CR_ReconnectTimeout, timeoutCTS.Token); + AsyncHelper.ContinueTask(reconnectTask, completion, + () => + { + if (completion.Task.IsCompleted) + { + return; + } + Interlocked.CompareExchange(ref _reconnectionCompletionSource, null, completion); + timeoutCTS.Cancel(); + Task subTask; + RunExecuteReaderTds(cmdBehavior, runBehavior, returnStream, async, TdsParserStaticMethods.GetRemainingTimeout(timeout, reconnectionStart), out subTask, asyncWrite, ds); + if (subTask == null) + { + completion.SetResult(null); + } + else + { + AsyncHelper.ContinueTask(subTask, completion, () => completion.SetResult(null)); + } + }, connectionToAbort: _activeConnection + ); + } private SqlDataReader CompleteAsyncExecuteReader() { diff --git a/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlConnection.cs b/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlConnection.cs index d23e602887ae..80ffa5bb1e40 100644 --- a/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlConnection.cs +++ b/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlConnection.cs @@ -912,7 +912,7 @@ internal Task ValidateAndReconnect(Action beforeDisconnect, int timeout) { } // use Task.Factory.StartNew with state overload instead of Task.Run to avoid anonymous closure context capture in method scope and avoid the unneeded allocation - runningReconnect = Task.Factory.StartNew(state => ReconnectAsync((int)state), timeout,CancellationToken.None, TaskCreationOptions.DenyChildAttach,TaskScheduler.Default); + runningReconnect = Task.Factory.StartNew(state => ReconnectAsync((int)state), timeout, CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default); // if current reconnect is not null, somebody already started reconnection task - some kind of race condition Debug.Assert(_currentReconnectionTask == null, "Duplicate reconnection tasks detected"); _currentReconnectionTask = runningReconnect; diff --git a/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlUtil.cs b/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlUtil.cs index 56f49f253980..2882b3ea6d62 100644 --- a/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlUtil.cs +++ b/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlUtil.cs @@ -34,11 +34,30 @@ internal static Task CreateContinuationTask(Task task, Action onSuccess, SqlInte } } + internal static Task CreateContinuationTaskWithState(Task task, object state, Action onSuccess, Action onFailure = null) + { + if (task == null) + { + onSuccess(state); + return null; + } + else + { + var completion = new TaskCompletionSource(); + ContinueTaskWithState(task, completion, state, + onSuccess: (continueState) => { onSuccess(continueState); completion.SetResult(null); }, + onFailure: onFailure + ); + return completion.Task; + } + } + internal static Task CreateContinuationTask(Task task, Action onSuccess, T1 arg1, T2 arg2, SqlInternalConnectionTds connectionToDoom = null, Action onFailure = null) { return CreateContinuationTask(task, () => onSuccess(arg1, arg2), connectionToDoom, onFailure); } + internal static void ContinueTask(Task task, TaskCompletionSource completion, Action onSuccess, @@ -101,6 +120,61 @@ internal static void ContinueTask(Task task, ); } + // the same logic as ContinueTask but with an added state parameter to allow the caller to avoid the use of a closure + // the parameter allocation cannot be avoided here and using closure names is clearer than Tuple numbered properties + internal static void ContinueTaskWithState(Task task, + TaskCompletionSource completion, + object state, + Action onSuccess, + Action onFailure = null, + Action onCancellation = null, + Func exceptionConverter = null + ) + { + task.ContinueWith( + tsk => + { + if (tsk.Exception != null) + { + Exception exc = tsk.Exception.InnerException; + if (exceptionConverter != null) + { + exc = exceptionConverter(exc); + } + try + { + onFailure?.Invoke(exc, state); + } + finally + { + completion.TrySetException(exc); + } + } + else if (tsk.IsCanceled) + { + try + { + onCancellation?.Invoke(state); + } + finally + { + completion.TrySetCanceled(); + } + } + else + { + try + { + onSuccess(state); + } + catch (Exception e) + { + completion.SetException(e); + } + } + }, TaskScheduler.Default + ); + } internal static void WaitForCompletion(Task task, int timeout, Action onTimeout = null, bool rethrowExceptions = true) { From 6c3495fcf07b41389f7cbc172f9f4b25ef8d0783 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Thu, 13 Dec 2018 20:24:12 +0000 Subject: [PATCH 8/8] address feedback and remove further BeginExecuteNonQuery closure --- .../src/System/Data/SqlClient/SqlCommand.cs | 5 ++- .../src/System/Data/SqlClient/TdsParser.cs | 38 +++++++++++-------- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlCommand.cs b/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlCommand.cs index 1ecd3f49039a..6416f49e4589 100644 --- a/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlCommand.cs +++ b/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlCommand.cs @@ -947,7 +947,10 @@ public IAsyncResult BeginExecuteNonQuery(AsyncCallback callback, object stateObj // Add callback after work is done to avoid overlapping Begin\End methods if (callback != null) { - completion.Task.ContinueWith((t) => callback(t), TaskScheduler.Default); + completion.Task.ContinueWith( + (task,state) => ((AsyncCallback)state)(task), + state: callback + ); } return completion.Task; diff --git a/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs b/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs index 6a01db834b80..14e997883644 100644 --- a/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs +++ b/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs @@ -2160,10 +2160,7 @@ internal bool TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataRead } // This is in its own method to avoid always allocating the lambda in TryRun - private static void TryRunSetupSpinWaitContinuation(TdsParserStateObject stateObj) - { - SpinWait.SpinUntil(() => !stateObj._attentionSending); - } + private static void TryRunSetupSpinWaitContinuation(TdsParserStateObject stateObj) => SpinWait.SpinUntil(() => !stateObj._attentionSending); private bool TryProcessEnvChange(int tokenLength, TdsParserStateObject stateObj, out SqlEnvChange[] sqlEnvChange) { @@ -6982,13 +6979,15 @@ internal Task TdsExecuteSQLBatch(string text, int timeout, SqlNotificationReques (task, state) => { Debug.Assert(!task.IsCanceled, "Task should not be canceled"); - var parameters = (Tuple)state; + var parameters = (Tuple)state; + TdsParser parser = parameters.Item1; TdsParserStateObject tdsParserStateObject = parameters.Item2; + SqlInternalConnectionTds internalConnectionTds = parameters.Item3; try { if (task.IsFaulted) { - parameters.Item1.FailureCleanup(tdsParserStateObject, task.Exception.InnerException); + parser.FailureCleanup(tdsParserStateObject, task.Exception.InnerException); throw task.Exception.InnerException; } else @@ -6998,13 +6997,10 @@ internal Task TdsExecuteSQLBatch(string text, int timeout, SqlNotificationReques } finally { - if (parameters.Item3) - { - parameters.Item4._parserLock.Release(); - } + internalConnectionTds?._parserLock.Release(); } }, - Tuple.Create(this, stateObj, taskReleaseConnectionLock, taskReleaseConnectionLock ? _connHandler : null), + Tuple.Create(this, stateObj, taskReleaseConnectionLock ? _connHandler : null), TaskScheduler.Default ); } @@ -7506,7 +7502,19 @@ internal Task TdsExecuteRPC(_SqlRPC[] rpcArray, int timeout, bool inSchema, SqlN task = completion.Task; } - TDSExecuteRPCParameterSetupWriteCompletion(rpcArray, timeout, inSchema, notificationRequest, stateObj, isCommandProc, sync, completion, ii, i, writeParamTask); + TDSExecuteRPCParameterSetupWriteCompletion( + rpcArray, + timeout, + inSchema, + notificationRequest, + stateObj, + isCommandProc, + sync, + completion, + ii, + i+1, + writeParamTask + ); // Take care of releasing the locks if (releaseConnectionLock) @@ -7606,7 +7614,7 @@ internal Task TdsExecuteRPC(_SqlRPC[] rpcArray, int timeout, bool inSchema, SqlN // This is in its own method to avoid always allocating the lambda in TDSExecuteRPCParameter - private void TDSExecuteRPCParameterSetupWriteCompletion(_SqlRPC[] rpcArray, int timeout, bool inSchema, SqlNotificationRequest notificationRequest, TdsParserStateObject stateObj, bool isCommandProc, bool sync, TaskCompletionSource completion, int ii, int i, Task writeParamTask) + private void TDSExecuteRPCParameterSetupWriteCompletion(_SqlRPC[] rpcArray, int timeout, bool inSchema, SqlNotificationRequest notificationRequest, TdsParserStateObject stateObj, bool isCommandProc, bool sync, TaskCompletionSource completion, int startRpc, int startParam, Task writeParamTask) { AsyncHelper.ContinueTask( writeParamTask, @@ -7620,8 +7628,8 @@ private void TDSExecuteRPCParameterSetupWriteCompletion(_SqlRPC[] rpcArray, int isCommandProc, sync, completion, - startRpc: ii, - startParam: i + 1 + startRpc, + startParam ), connectionToDoom: _connHandler, onFailure: exc => TdsExecuteRPC_OnFailure(exc, stateObj)