From da9c8133a55679d68653e90d1985db0f55fec74a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=A2=D0=B0=D1=82=D1=8C=D1=8F=D0=BD=D0=B0=20=D0=93=D0=BB?= =?UTF-8?q?=D0=B0=D0=B4=D1=8B=D1=88=D0=B5=D0=B2=D0=B0?= Date: Thu, 28 Mar 2024 14:35:21 +0500 Subject: [PATCH 1/2] Don't use infinite connection timeout in strategies. --- .../Strategies/ForkingRequestStrategy_Tests.cs | 18 ------------------ .../ParallelRequestStrategy_Tests.cs | 4 ++-- .../SequentialRequestStrategy_Tests.cs | 4 ++-- .../SingleReplicaRequestStrategy_Tests.cs | 15 ++------------- .../Strategies/ForkingRequestStrategy.cs | 4 +--- .../Strategies/ParallelRequestStrategy.cs | 2 +- .../Strategies/SequentialRequestStrategy.cs | 4 +--- .../Strategies/SingleReplicaRequestStrategy.cs | 2 +- 8 files changed, 10 insertions(+), 43 deletions(-) diff --git a/Vostok.ClusterClient.Core.Tests/Strategies/ForkingRequestStrategy_Tests.cs b/Vostok.ClusterClient.Core.Tests/Strategies/ForkingRequestStrategy_Tests.cs index 6bd3a576..fb5edd53 100644 --- a/Vostok.ClusterClient.Core.Tests/Strategies/ForkingRequestStrategy_Tests.cs +++ b/Vostok.ClusterClient.Core.Tests/Strategies/ForkingRequestStrategy_Tests.cs @@ -210,24 +210,6 @@ public void Should_launch_parallel_requests_with_correct_parameters() sender.Received(1).SendToReplicaAsync(replicas[2], Arg.Any(), parameters.ConnectionTimeout, 5.Seconds(), Arg.Any()); } - [Test] - public void Should_launch_requests_except_last_with_connection_timeout() - { - sender.ClearReceivedCalls(); - - strategy = new ForkingRequestStrategy(delaysProvider, delaysPlanner, replicas.Length); - - strategy.SendAsync(request, parameters, sender, Budget.WithRemaining(5.Seconds()), replicas, replicas.Length, token); - - for (var i = 0; i < replicas.Length; ++i) - CompleteForkingDelay(); - - for (var i = 0; i < replicas.Length - 1; ++i) - sender.Received(1).SendToReplicaAsync(replicas[i], Arg.Any(), parameters.ConnectionTimeout, Arg.Any(), Arg.Any()); - - sender.Received(1).SendToReplicaAsync(replicas.Last(), Arg.Any(), null, Arg.Any(), Arg.Any()); - } - [TestCase(0)] [TestCase(1)] [TestCase(2)] diff --git a/Vostok.ClusterClient.Core.Tests/Strategies/ParallelRequestStrategy_Tests.cs b/Vostok.ClusterClient.Core.Tests/Strategies/ParallelRequestStrategy_Tests.cs index 7e82a16e..12a1f7f5 100644 --- a/Vostok.ClusterClient.Core.Tests/Strategies/ParallelRequestStrategy_Tests.cs +++ b/Vostok.ClusterClient.Core.Tests/Strategies/ParallelRequestStrategy_Tests.cs @@ -146,7 +146,7 @@ public void Should_fire_initial_requests_to_all_replicas_if_parallelism_level_is } [Test] - public void Should_ignore_connection_timeout() + public void Should_use_configured_connection_timeout_for_all_requests() { strategy = new ParallelRequestStrategy(int.MaxValue); @@ -158,7 +158,7 @@ public void Should_ignore_connection_timeout() foreach (var replica in replicas) { - sender.Received(1).SendToReplicaAsync(replica, Arg.Any(), null, Arg.Any(), Arg.Any()); + sender.Received(1).SendToReplicaAsync(replica, Arg.Any(), parameters.ConnectionTimeout, Arg.Any(), Arg.Any()); } } diff --git a/Vostok.ClusterClient.Core.Tests/Strategies/SequentialRequestStrategy_Tests.cs b/Vostok.ClusterClient.Core.Tests/Strategies/SequentialRequestStrategy_Tests.cs index 8eef14db..7db6ba11 100644 --- a/Vostok.ClusterClient.Core.Tests/Strategies/SequentialRequestStrategy_Tests.cs +++ b/Vostok.ClusterClient.Core.Tests/Strategies/SequentialRequestStrategy_Tests.cs @@ -145,14 +145,14 @@ public void Should_limit_request_timeouts_by_remaining_time_budget() } [Test] - public void Should_not_use_connection_timeout_for_last_attempt() + public void Should_use_configured_connection_timeout() { Send(Budget.WithRemaining(1500.Milliseconds())); sender.ReceivedCalls().Should().HaveCount(3); sender.Received(1).SendToReplicaAsync(replica1, request, parameters.ConnectionTimeout, Arg.Any(), token); sender.Received(1).SendToReplicaAsync(replica2, request, parameters.ConnectionTimeout, Arg.Any(), token); - sender.Received(1).SendToReplicaAsync(replica3, request, null, Arg.Any(), token); + sender.Received(1).SendToReplicaAsync(replica3, request, parameters.ConnectionTimeout, Arg.Any(), token); } [Test] diff --git a/Vostok.ClusterClient.Core.Tests/Strategies/SingleReplicaRequestStrategy_Tests.cs b/Vostok.ClusterClient.Core.Tests/Strategies/SingleReplicaRequestStrategy_Tests.cs index 70968835..f0bc4c20 100644 --- a/Vostok.ClusterClient.Core.Tests/Strategies/SingleReplicaRequestStrategy_Tests.cs +++ b/Vostok.ClusterClient.Core.Tests/Strategies/SingleReplicaRequestStrategy_Tests.cs @@ -49,20 +49,9 @@ public void Should_send_request_to_first_replica_with_correct_parameters() strategy.SendAsync(request, parameters, sender, budget, replicas, replicas.Length, token).Wait(); - sender.Received().SendToReplicaAsync(replicas[0], request, null, budget.Remaining, token); - } - - [Test] - public void Should_ignore_connection_timeout() - { - parameters = parameters.WithConnectionTimeout(5.Seconds()); - - var token = new CancellationTokenSource().Token; - - strategy.SendAsync(request, parameters, sender, budget, replicas, replicas.Length, token).Wait(); - - sender.Received().SendToReplicaAsync(Arg.Any(), Arg.Any(), null, Arg.Any(), Arg.Any()); + sender.Received().SendToReplicaAsync(replicas[0], request, parameters.ConnectionTimeout, budget.Remaining, token); } + [Test] public void Should_stop_on_first_result_if_its_response_is_accepted() diff --git a/Vostok.ClusterClient.Core/Strategies/ForkingRequestStrategy.cs b/Vostok.ClusterClient.Core/Strategies/ForkingRequestStrategy.cs index 539a4278..9a0d0e94 100644 --- a/Vostok.ClusterClient.Core/Strategies/ForkingRequestStrategy.cs +++ b/Vostok.ClusterClient.Core/Strategies/ForkingRequestStrategy.cs @@ -83,9 +83,7 @@ public async Task SendAsync(Request request, RequestParameters parameters, IRequ break; } - var connectionAttemptTimeout = i == replicasCount - 1 ? null : parameters.ConnectionTimeout; - - LaunchRequest(currentTasks, request, budget, sender, replicasEnumerator, connectionAttemptTimeout, linkedCancellationToken); + LaunchRequest(currentTasks, request, budget, sender, replicasEnumerator, parameters.ConnectionTimeout, linkedCancellationToken); ScheduleForkIfNeeded(currentTasks, request, budget, i, replicasCount, linkedCancellationToken); diff --git a/Vostok.ClusterClient.Core/Strategies/ParallelRequestStrategy.cs b/Vostok.ClusterClient.Core/Strategies/ParallelRequestStrategy.cs index 50dedf96..c56046a0 100644 --- a/Vostok.ClusterClient.Core/Strategies/ParallelRequestStrategy.cs +++ b/Vostok.ClusterClient.Core/Strategies/ParallelRequestStrategy.cs @@ -59,7 +59,7 @@ public async Task SendAsync(Request request, RequestParameters parameters, IRequ if (!replicasEnumerator.MoveNext()) throw new InvalidOperationException("Replicas enumerator ended prematurely. This is definitely a bug in code."); - currentTasks.Add(sender.SendToReplicaAsync(replicasEnumerator.Current, request, null, budget.Remaining, linkedCancellationToken)); + currentTasks.Add(sender.SendToReplicaAsync(replicasEnumerator.Current, request, parameters.ConnectionTimeout, budget.Remaining, linkedCancellationToken)); } while (currentTasks.Count > 0) diff --git a/Vostok.ClusterClient.Core/Strategies/SequentialRequestStrategy.cs b/Vostok.ClusterClient.Core/Strategies/SequentialRequestStrategy.cs index 83b620a4..0c262d92 100644 --- a/Vostok.ClusterClient.Core/Strategies/SequentialRequestStrategy.cs +++ b/Vostok.ClusterClient.Core/Strategies/SequentialRequestStrategy.cs @@ -53,9 +53,7 @@ public async Task SendAsync(Request request, RequestParameters parameters, IRequ var timeout = TimeSpanArithmetics.Min(timeoutsProvider.GetTimeout(request, budget, currentReplicaIndex++, replicasCount), budget.Remaining); - var connectionAttemptTimeout = currentReplicaIndex == replicasCount ? null : parameters.ConnectionTimeout; - - var result = await sender.SendToReplicaAsync(replica, request, connectionAttemptTimeout, timeout, cancellationToken).ConfigureAwait(false); + var result = await sender.SendToReplicaAsync(replica, request, parameters.ConnectionTimeout, timeout, cancellationToken).ConfigureAwait(false); if (result.Verdict == ResponseVerdict.Accept) break; diff --git a/Vostok.ClusterClient.Core/Strategies/SingleReplicaRequestStrategy.cs b/Vostok.ClusterClient.Core/Strategies/SingleReplicaRequestStrategy.cs index 2b80f2c6..8c7f3428 100644 --- a/Vostok.ClusterClient.Core/Strategies/SingleReplicaRequestStrategy.cs +++ b/Vostok.ClusterClient.Core/Strategies/SingleReplicaRequestStrategy.cs @@ -23,7 +23,7 @@ public class SingleReplicaRequestStrategy : IRequestStrategy { /// public Task SendAsync(Request request, RequestParameters parameters, IRequestSender sender, IRequestTimeBudget budget, IEnumerable replicas, int replicasCount, CancellationToken cancellationToken) => - sender.SendToReplicaAsync(replicas.First(), request, null, budget.Remaining, cancellationToken); + sender.SendToReplicaAsync(replicas.First(), request, parameters.ConnectionTimeout, budget.Remaining, cancellationToken); /// public override string ToString() => "SingleReplica"; From 374a3e14ae92e523adfcd7ac4a8999eeb427249a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=A2=D0=B0=D1=82=D1=8C=D1=8F=D0=BD=D0=B0=20=D0=93=D0=BB?= =?UTF-8?q?=D0=B0=D0=B4=D1=8B=D1=88=D0=B5=D0=B2=D0=B0?= Date: Thu, 28 Mar 2024 14:59:50 +0500 Subject: [PATCH 2/2] Add test for forking strategy --- .../ForkingRequestStrategy_Tests.cs | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/Vostok.ClusterClient.Core.Tests/Strategies/ForkingRequestStrategy_Tests.cs b/Vostok.ClusterClient.Core.Tests/Strategies/ForkingRequestStrategy_Tests.cs index fb5edd53..0be57d12 100644 --- a/Vostok.ClusterClient.Core.Tests/Strategies/ForkingRequestStrategy_Tests.cs +++ b/Vostok.ClusterClient.Core.Tests/Strategies/ForkingRequestStrategy_Tests.cs @@ -210,6 +210,24 @@ public void Should_launch_parallel_requests_with_correct_parameters() sender.Received(1).SendToReplicaAsync(replicas[2], Arg.Any(), parameters.ConnectionTimeout, 5.Seconds(), Arg.Any()); } + [Test] + public void Should_launch_all_requests_with_configured_connection_timeout() + { + sender.ClearReceivedCalls(); + + strategy = new ForkingRequestStrategy(delaysProvider, delaysPlanner, replicas.Length); + + strategy.SendAsync(request, parameters, sender, Budget.WithRemaining(5.Seconds()), replicas, replicas.Length, token); + + for (var i = 0; i < replicas.Length; ++i) + CompleteForkingDelay(); + + for (var i = 0; i < replicas.Length - 1; ++i) + sender.Received(1).SendToReplicaAsync(replicas[i], Arg.Any(), parameters.ConnectionTimeout, Arg.Any(), Arg.Any()); + + sender.Received(1).SendToReplicaAsync(replicas.Last(), Arg.Any(), parameters.ConnectionTimeout, Arg.Any(), Arg.Any()); + } + [TestCase(0)] [TestCase(1)] [TestCase(2)] @@ -225,7 +243,7 @@ public void Should_stop_when_any_of_requests_completes_with_accepted_result(int } [Test] - public void Should_issue_another_request_when_a_pending_one_ends_with_rejected_status([Values]bool unreliableHeaderPresent) + public void Should_issue_another_request_when_a_pending_one_ends_with_rejected_status([Values] bool unreliableHeaderPresent) { var task = strategy.SendAsync(request, parameters, sender, Budget.Infinite, replicas, replicas.Length, token);