From 8412c78091733785d5430fadd14496ce072cce4e Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Fri, 20 Aug 2021 15:17:38 -0700 Subject: [PATCH] Fix multiple retry issue on ExecuteScalarAsync + add missing ConfigureAwait calls (#1220) --- .../Microsoft/Data/SqlClient/SqlCommand.cs | 22 +- .../Microsoft/Data/SqlClient/SqlCommand.cs | 10 +- .../Common/SqlRetryLogicProvider.cs | 19 +- ....Data.SqlClient.ManualTesting.Tests.csproj | 1 + .../SQL/RetryLogic/RetryLogicCounterTest.cs | 198 ++++++++++++++++++ 5 files changed, 220 insertions(+), 30 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/RetryLogic/RetryLogicCounterTest.cs diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs index 3975c5d3ca..2d36d0812d 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs @@ -2575,9 +2575,9 @@ protected override Task ExecuteDbDataReaderAsync(CommandBehavior b throw result.Exception.InnerException; } return result.Result; - }, - CancellationToken.None, - TaskContinuationOptions.ExecuteSynchronously | TaskContinuationOptions.NotOnCanceled, + }, + CancellationToken.None, + TaskContinuationOptions.ExecuteSynchronously | TaskContinuationOptions.NotOnCanceled, TaskScheduler.Default ); } @@ -2679,13 +2679,9 @@ private void SetCachedCommandExecuteReaderAsyncContext(ExecuteReaderAsyncCallCon } /// - public override Task ExecuteScalarAsync(CancellationToken cancellationToken) - => IsRetryEnabled ? - InternalExecuteScalarWithRetryAsync(cancellationToken) : - InternalExecuteScalarAsync(cancellationToken); - - private Task InternalExecuteScalarWithRetryAsync(CancellationToken cancellationToken) - => RetryLogicProvider.ExecuteAsync(this, () => InternalExecuteScalarAsync(cancellationToken), cancellationToken); + public override Task ExecuteScalarAsync(CancellationToken cancellationToken) => + // Do not use retry logic here as internal call to ExecuteReaderAsync handles retry logic. + InternalExecuteScalarAsync(cancellationToken); private Task InternalExecuteScalarAsync(CancellationToken cancellationToken) { @@ -3733,13 +3729,13 @@ private SqlDataReader GetParameterEncryptionDataReader(out Task returnTask, Task // Read the results of describe parameter encryption. command.ReadDescribeEncryptionParameterResults(describeParameterEncryptionDataReader, describeParameterEncryptionRpcOriginalRpcMap); - #if DEBUG +#if DEBUG // Failpoint to force the thread to halt to simulate cancellation of SqlCommand. if (_sleepAfterReadDescribeEncryptionParameterResults) { Thread.Sleep(10000); } - #endif +#endif } catch (Exception e) { @@ -4897,7 +4893,7 @@ private SqlDataReader RunExecuteReaderTds(CommandBehavior cmdBehavior, RunBehavi private Task RunExecuteReaderTdsSetupContinuation(RunBehavior runBehavior, SqlDataReader ds, string optionSettings, Task writeTask) { Task task = AsyncHelper.CreateContinuationTaskWithState( - task: writeTask, + task: writeTask, state: _activeConnection, onSuccess: (object state) => { diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs index 76bfed9986..168584e9d4 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs @@ -3106,14 +3106,10 @@ private Task InternalExecuteReaderAsync(CommandBehavior behavior, return returnedTask; } - private Task InternalExecuteScalarWithRetryAsync(CancellationToken cancellationToken) - => RetryLogicProvider.ExecuteAsync(this, () => InternalExecuteScalarAsync(cancellationToken), cancellationToken); - /// - public override Task ExecuteScalarAsync(CancellationToken cancellationToken) - => IsRetryEnabled ? - InternalExecuteScalarWithRetryAsync(cancellationToken) : - InternalExecuteScalarAsync(cancellationToken); + public override Task ExecuteScalarAsync(CancellationToken cancellationToken) => + // Do not use retry logic here as internal call to ExecuteReaderAsync handles retry logic. + InternalExecuteScalarAsync(cancellationToken); private Task InternalExecuteScalarAsync(CancellationToken cancellationToken) { diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Reliability/Common/SqlRetryLogicProvider.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Reliability/Common/SqlRetryLogicProvider.cs index c865c2870c..758e6fe2be 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Reliability/Common/SqlRetryLogicProvider.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Reliability/Common/SqlRetryLogicProvider.cs @@ -17,7 +17,7 @@ internal class SqlRetryLogicProvider : SqlRetryLogicBaseProvider { private const string TypeName = nameof(SqlRetryLogicProvider); // keeps free RetryLogic objects - private readonly ConcurrentBag _retryLogicPool = new ConcurrentBag(); + private readonly ConcurrentBag _retryLogicPool = new(); /// Creates an instance of this type. public SqlRetryLogicProvider(SqlRetryLogicBase retryLogic) @@ -28,8 +28,7 @@ public SqlRetryLogicProvider(SqlRetryLogicBase retryLogic) private SqlRetryLogicBase GetRetryLogic() { - SqlRetryLogicBase retryLogic = null; - if (!_retryLogicPool.TryTake(out retryLogic)) + if (!_retryLogicPool.TryTake(out SqlRetryLogicBase retryLogic)) { retryLogic = RetryLogic.Clone() as SqlRetryLogicBase; } @@ -69,7 +68,7 @@ public override TResult Execute(object sender, Func function) { if (RetryLogic.RetryCondition(sender) && RetryLogic.TransientPredicate(e)) { - retryLogic = retryLogic ?? GetRetryLogic(); + retryLogic ??= GetRetryLogic(); SqlClientEventSource.Log.TryTraceEvent("|INFO> Found an action eligible for the retry policy (retried attempts = {1}).", TypeName, retryLogic.Current); exceptions.Add(e); @@ -107,7 +106,7 @@ public override async Task ExecuteAsync(object sender, Func ExecuteAsync(object sender, Func|INFO> Found an action eligible for the retry policy (retried attempts = {1}).", TypeName, retryLogic.Current); exceptions.Add(e); @@ -124,7 +123,7 @@ public override async Task ExecuteAsync(object sender, Func function, Canc retry: try { - await function.Invoke(); + await function.Invoke().ConfigureAwait(false); RetryLogicPoolAdd(retryLogic); } catch (Exception e) { if (RetryLogic.RetryCondition(sender) && RetryLogic.TransientPredicate(e)) { - retryLogic = retryLogic ?? GetRetryLogic(); + retryLogic ??= GetRetryLogic(); SqlClientEventSource.Log.TryTraceEvent(" Found an action eligible for the retry policy (retried attempts = {1}).", TypeName, retryLogic.Current); exceptions.Add(e); @@ -169,7 +168,7 @@ public override async Task ExecuteAsync(object sender, Func function, Canc // The retrying event raises on each retry. ApplyRetryingEvent(sender, retryLogic, intervalTime, exceptions, e); - await Task.Delay(intervalTime, cancellationToken); + await Task.Delay(intervalTime, cancellationToken).ConfigureAwait(false); goto retry; } else diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj b/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj index ec16e4f34b..a308ef7520 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj @@ -66,6 +66,7 @@ + diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/RetryLogic/RetryLogicCounterTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/RetryLogic/RetryLogicCounterTest.cs new file mode 100644 index 0000000000..86f53151bd --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/RetryLogic/RetryLogicCounterTest.cs @@ -0,0 +1,198 @@ +// 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.Threading; +using System.Threading.Tasks; +using Xunit; + +namespace Microsoft.Data.SqlClient.ManualTesting.Tests +{ + public class RetryLogicCounterTest + { + [ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] + [InlineData("ExecuteScalarAsync", 3)] + [InlineData("ExecuteReaderAsync", 3)] + [InlineData("ExecuteXmlReaderAsync", 3)] + [InlineData("ExecuteNonQueryAsync", 3)] + public async void ValidateRetryCount_SqlCommand_Async(string methodName, int numOfTries) + { + ErrorInfoRetryLogicProvider _errorInfoRetryProvider = new( + SqlConfigurableRetryFactory.CreateFixedRetryProvider(new SqlRetryLogicOption() + { NumberOfTries = numOfTries, TransientErrors = new[] { 50000 } })); + + try + { + RetryLogicTestHelper.SetRetrySwitch(true); + + using var connection = new SqlConnection(DataTestUtility.TCPConnectionString); + connection.Open(); + + using SqlCommand cmd = connection.CreateCommand(); + cmd.RetryLogicProvider = _errorInfoRetryProvider; + cmd.CommandText = "THROW 50000,'Error',0"; + + _errorInfoRetryProvider.CallCounter = 0; + switch (methodName) + { + case "ExecuteScalarAsync": + await cmd.ExecuteScalarAsync(); + break; + case "ExecuteReaderAsync": + { + using SqlDataReader _ = await cmd.ExecuteReaderAsync(); + break; + } + case "ExecuteXmlReaderAsync": + { + using System.Xml.XmlReader _ = await cmd.ExecuteXmlReaderAsync(); + break; + } + case "ExecuteNonQueryAsync": + await cmd.ExecuteNonQueryAsync(); + break; + default: + break; + } + Assert.False(true, "Exception did not occur."); + } + catch + { + Assert.Equal(numOfTries, _errorInfoRetryProvider.CallCounter); + } + finally + { + RetryLogicTestHelper.SetRetrySwitch(false); + } + } + + [ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] + [InlineData("ExecuteScalar", 3)] + [InlineData("ExecuteReader", 3)] + [InlineData("ExecuteXmlReader", 3)] + [InlineData("ExecuteNonQuery", 3)] + public void ValidateRetryCount_SqlCommand_Sync(string methodName, int numOfTries) + { + ErrorInfoRetryLogicProvider _errorInfoRetryProvider = new( + SqlConfigurableRetryFactory.CreateFixedRetryProvider(new SqlRetryLogicOption() + { NumberOfTries = numOfTries, TransientErrors = new[] { 50000 } })); + + try + { + RetryLogicTestHelper.SetRetrySwitch(true); + + using var connection = new SqlConnection(DataTestUtility.TCPConnectionString); + connection.Open(); + + using SqlCommand cmd = connection.CreateCommand(); + cmd.RetryLogicProvider = _errorInfoRetryProvider; + cmd.CommandText = "THROW 50000,'Error',0"; + + _errorInfoRetryProvider.CallCounter = 0; + switch (methodName) + { + case "ExecuteScalar": + cmd.ExecuteScalar(); + break; + case "ExecuteReader": + { + using SqlDataReader _ = cmd.ExecuteReader(); + break; + } + case "ExecuteXmlReader": + { + using System.Xml.XmlReader _ = cmd.ExecuteXmlReader(); + break; + } + case "ExecuteNonQuery": + cmd.ExecuteNonQuery(); + break; + default: + break; + } + Assert.False(true, "Exception did not occur."); + } + catch + { + Assert.Equal(numOfTries, _errorInfoRetryProvider.CallCounter); + } + finally + { + RetryLogicTestHelper.SetRetrySwitch(false); + } + } + + public class ErrorInfoRetryLogicProvider : SqlRetryLogicBaseProvider + { + public SqlRetryLogicBaseProvider InnerProvider { get; } + + public ErrorInfoRetryLogicProvider(SqlRetryLogicBaseProvider innerProvider) + { + InnerProvider = innerProvider; + } + + readonly AsyncLocal _depth = new(); + public int CallCounter = 0; + + TResult LogCalls(Func function) + { + CallCounter++; + return function(); + } + + public override TResult Execute(object sender, Func function) + { + _depth.Value++; + try + { + return InnerProvider.Execute(sender, () => LogCalls(function)); + } + finally + { + _depth.Value--; + } + } + + public async Task LogCallsAsync(Func> function) + { + CallCounter++; + return await function(); + } + + public override async Task ExecuteAsync(object sender, Func> function, + CancellationToken cancellationToken = default) + { + _depth.Value++; + try + { + return await InnerProvider.ExecuteAsync(sender, () => LogCallsAsync(function), cancellationToken); + } + finally + { + _depth.Value--; + } + } + + public async Task LogCallsAsync(Func function) + { + CallCounter++; + await function(); + } + + public override async Task ExecuteAsync(object sender, Func function, + CancellationToken cancellationToken = default) + { + _depth.Value++; + try + { + await InnerProvider.ExecuteAsync(sender, () => LogCallsAsync(function), cancellationToken); + } + finally + { + _depth.Value--; + } + } + } + } +}