Skip to content

Commit

Permalink
PR feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@JamesNK
JamesNK committed Feb 19, 2021
1 parent 0089981 commit 5df6bf2
Show file tree
Hide file tree
Showing 9 changed files with 157 additions and 87 deletions.
2 changes: 1 addition & 1 deletion src/Grpc.Net.Client/Internal/GrpcCall.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public GrpcCall(Method<TRequest, TResponse> method, GrpcMethodInfo grpcMethodInf
Channel.RegisterActiveCall(this);
}

public MethodConfig? MethodConfig => _grpcMethodInfo.MethodConfig;
public MethodConfigInfo? MethodConfig => _grpcMethodInfo.MethodConfig;

private void ValidateDeadline(DateTime? deadline)
{
Expand Down
105 changes: 103 additions & 2 deletions src/Grpc.Net.Client/Internal/GrpcMethodInfo.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
#endregion

using System;
using System.Collections.Generic;
using System.Linq;
using Grpc.Core;
using Grpc.Net.Client.Configuration;

Expand All @@ -31,11 +33,110 @@ public GrpcMethodInfo(GrpcCallScope logScope, Uri callUri, MethodConfig? methodC
{
LogScope = logScope;
CallUri = callUri;
MethodConfig = methodConfig;
MethodConfig = CreateMethodConfig(methodConfig);
}

private MethodConfigInfo? CreateMethodConfig(MethodConfig? methodConfig)
{
if (methodConfig == null)
{
return null;
}
if (methodConfig.RetryPolicy != null && methodConfig.HedgingPolicy != null)
{
throw new InvalidOperationException("Method config can't have a retry policy and hedging policy.");
}

MethodConfigInfo m = new MethodConfigInfo();

if (methodConfig.RetryPolicy is { } r)
{
if (!(r.MaxAttempts > 1))
{
throw new InvalidOperationException("Retry policy max attempts must be greater than 1.");
}
if (!(r.InitialBackoff > TimeSpan.Zero))
{
throw new InvalidOperationException("Retry policy initial backoff must be greater than zero.");
}
if (!(r.MaxBackoff > TimeSpan.Zero))
{
throw new InvalidOperationException("Retry policy maximum backoff must be greater than zero.");
}
if (!(r.BackoffMultiplier > 0))
{
throw new InvalidOperationException("Retry policy backoff multiplier must be greater than 0.");
}
if (!(r.RetryableStatusCodes.Count > 0))
{
throw new InvalidOperationException("Retry policy must specify at least 1 retryable status code.");
}

m.RetryPolicy = new RetryPolicyInfo
{
MaxAttempts = r.MaxAttempts.GetValueOrDefault(),
InitialBackoff = r.InitialBackoff.GetValueOrDefault(),
MaxBackoff = r.MaxBackoff.GetValueOrDefault(),
BackoffMultiplier = r.BackoffMultiplier.GetValueOrDefault(),
RetryableStatusCodes = r.RetryableStatusCodes.ToList()
};
}

if (methodConfig.HedgingPolicy != null)
{
m.HedgingPolicy = CreateHedgingPolicy(methodConfig.HedgingPolicy);
}

return m;
}

internal static HedgingPolicyInfo CreateHedgingPolicy(HedgingPolicy h)
{
if (!(h.MaxAttempts > 1))
{
throw new InvalidOperationException("Hedging policy max attempts must be greater than 1.");
}
if (!(h.HedgingDelay >= TimeSpan.Zero))
{
throw new InvalidOperationException("Hedging policy delay must be equal or greater than zero.");
}
if (!(h.NonFatalStatusCodes.Count > 0))
{
throw new InvalidOperationException("Hedging policy must specify at least 1 non-fatal status code.");
}

return new HedgingPolicyInfo
{
MaxAttempts = h.MaxAttempts.GetValueOrDefault(),
HedgingDelay = h.HedgingDelay.GetValueOrDefault(),
NonFatalStatusCodes = h.NonFatalStatusCodes.ToList()
};
}

public GrpcCallScope LogScope { get; }
public Uri CallUri { get; }
public MethodConfig? MethodConfig { get; }
public MethodConfigInfo? MethodConfig { get; }
}

internal class MethodConfigInfo
{
public RetryPolicyInfo? RetryPolicy { get; set; }
public HedgingPolicyInfo? HedgingPolicy { get; set; }
}

internal class RetryPolicyInfo
{
public int MaxAttempts { get; init; }
public TimeSpan InitialBackoff { get; init; }
public TimeSpan MaxBackoff { get; init; }
public double BackoffMultiplier { get; init; }
public List<StatusCode> RetryableStatusCodes { get; init; } = default!;
}

internal class HedgingPolicyInfo
{
public int MaxAttempts { get; set; }
public TimeSpan HedgingDelay { get; set; }
public List<StatusCode> NonFatalStatusCodes { get; init; } = default!;
}
}
29 changes: 29 additions & 0 deletions src/Grpc.Net.Client/Internal/IsExternalInit.cs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
#region Copyright notice and license

// Copyright 2019 The gRPC Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#endregion

using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;

namespace System.Runtime.CompilerServices
{
// Required for init properties in netstandard2.0
[ExcludeFromCodeCoverage, DebuggerNonUserCode]
internal static class IsExternalInit
{
}
}
41 changes: 5 additions & 36 deletions src/Grpc.Net.Client/Internal/Retry/HedgingCall.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ internal sealed partial class HedgingCall<TRequest, TResponse> : RetryCallBase<T
// Getting logger name from generic type is slow. Cached copy.
private const string LoggerName = "Grpc.Net.Client.Internal.HedgingCall";

private readonly HedgingPolicy _hedgingPolicy;
private readonly HedgingPolicyInfo _hedgingPolicy;

private int _callsAttempted;

Expand All @@ -46,8 +46,8 @@ internal sealed partial class HedgingCall<TRequest, TResponse> : RetryCallBase<T
internal List<IGrpcCall<TRequest, TResponse>> _activeCalls { get; }
internal Task? CreateHedgingCallsTask { get; set; }

public HedgingCall(HedgingPolicy hedgingPolicy, GrpcChannel channel, Method<TRequest, TResponse> method, CallOptions options)
: base(channel, method, options, LoggerName, hedgingPolicy.MaxAttempts.GetValueOrDefault())
public HedgingCall(HedgingPolicyInfo hedgingPolicy, GrpcChannel channel, Method<TRequest, TResponse> method, CallOptions options)
: base(channel, method, options, LoggerName, hedgingPolicy.MaxAttempts)
{
_hedgingPolicy = hedgingPolicy;
_activeCalls = new List<IGrpcCall<TRequest, TResponse>>();
Expand All @@ -56,37 +56,6 @@ public HedgingCall(HedgingPolicy hedgingPolicy, GrpcChannel channel, Method<TReq
{
_pushbackReceivedTcs = new TaskCompletionSource<object?>(TaskCreationOptions.None);
}

ValidatePolicy(hedgingPolicy);
}

private void ValidatePolicy(HedgingPolicy hedgingPolicy)
{
//if (retryThrottlingPolicy.MaxAttempts == null)
//{
// throw CreateException(_method, RetryPolicy.MaxAttemptsPropertyName);
//}
//if (retryThrottlingPolicy.InitialBackoff == null)
//{
// throw CreateException(_method, RetryPolicy.InitialBackoffPropertyName);
//}
//if (retryThrottlingPolicy.MaxBackoff == null)
//{
// throw CreateException(_method, RetryPolicy.MaxBackoffPropertyName);
//}
//if (retryThrottlingPolicy.BackoffMultiplier == null)
//{
// throw CreateException(_method, RetryPolicy.BackoffMultiplierPropertyName);
//}
//if (retryThrottlingPolicy.RetryableStatusCodes.Count == 0)
//{
// throw new InvalidOperationException($"Retry policy for '{_method.FullName}' must have property '{RetryPolicy.RetryableStatusCodesPropertyName}' and must be non-empty.");
//}

//static InvalidOperationException CreateException(IMethod method, string propertyName)
//{
// return new InvalidOperationException($"Retry policy for '{method.FullName}' is missing required property '{propertyName}'.");
//}
}

private async Task StartCall(Action<GrpcCall<TRequest, TResponse>> startCallFunc)
Expand Down Expand Up @@ -222,7 +191,7 @@ private void CleanUpUnsynchronized()

protected override void StartCore(Action<GrpcCall<TRequest, TResponse>> startCallFunc)
{
var hedgingDelay = _hedgingPolicy.HedgingDelay.GetValueOrDefault();
var hedgingDelay = _hedgingPolicy.HedgingDelay;
if (hedgingDelay == TimeSpan.Zero)
{
// If there is no delay then start all call immediately
Expand Down Expand Up @@ -259,7 +228,7 @@ private async Task CreateHedgingCalls(Action<GrpcCall<TRequest, TResponse>> star

try
{
var hedgingDelay = _hedgingPolicy.HedgingDelay.GetValueOrDefault();
var hedgingDelay = _hedgingPolicy.HedgingDelay;

while (_callsAttempted < MaxRetryAttempts)
{
Expand Down
43 changes: 6 additions & 37 deletions src/Grpc.Net.Client/Internal/Retry/RetryCall.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ internal sealed class RetryCall<TRequest, TResponse> : RetryCallBase<TRequest, T
// Getting logger name from generic type is slow. Cached copy.
private const string LoggerName = "Grpc.Net.Client.Internal.RetryCall";

private readonly RetryPolicy _retryPolicy;
private readonly RetryPolicyInfo _retryPolicy;

private readonly Random _random;

Expand All @@ -43,51 +43,20 @@ internal sealed class RetryCall<TRequest, TResponse> : RetryCallBase<TRequest, T

private GrpcCall<TRequest, TResponse>? _activeCall;

public RetryCall(RetryPolicy retryPolicy, GrpcChannel channel, Method<TRequest, TResponse> method, CallOptions options)
: base(channel, method, options, LoggerName, retryPolicy.MaxAttempts.GetValueOrDefault())
public RetryCall(RetryPolicyInfo retryPolicy, GrpcChannel channel, Method<TRequest, TResponse> method, CallOptions options)
: base(channel, method, options, LoggerName, retryPolicy.MaxAttempts)
{
_retryPolicy = retryPolicy;

_random = new Random();

ValidatePolicy(retryPolicy);

_nextRetryDelayMilliseconds = Convert.ToInt32(retryPolicy.InitialBackoff.GetValueOrDefault().TotalMilliseconds);
}

private void ValidatePolicy(RetryPolicy retryPolicy)
{
if (retryPolicy.MaxAttempts == null)
{
throw CreateException(Method, RetryPolicy.MaxAttemptsPropertyName);
}
if (retryPolicy.InitialBackoff == null)
{
throw CreateException(Method, RetryPolicy.InitialBackoffPropertyName);
}
if (retryPolicy.MaxBackoff == null)
{
throw CreateException(Method, RetryPolicy.MaxBackoffPropertyName);
}
if (retryPolicy.BackoffMultiplier == null)
{
throw CreateException(Method, RetryPolicy.BackoffMultiplierPropertyName);
}
if (retryPolicy.RetryableStatusCodes.Count == 0)
{
throw new InvalidOperationException($"Retry policy for '{Method.FullName}' must have property '{RetryPolicy.RetryableStatusCodesPropertyName}' and must be non-empty.");
}

static InvalidOperationException CreateException(IMethod method, string propertyName)
{
return new InvalidOperationException($"Retry policy for '{method.FullName}' is missing required property '{propertyName}'.");
}
_nextRetryDelayMilliseconds = Convert.ToInt32(retryPolicy.InitialBackoff.TotalMilliseconds);
}

private int CalculateNextRetryDelay()
{
var nextMilliseconds = _nextRetryDelayMilliseconds * _retryPolicy.BackoffMultiplier.GetValueOrDefault();
nextMilliseconds = Math.Min(nextMilliseconds, _retryPolicy.MaxBackoff.GetValueOrDefault().TotalMilliseconds);
var nextMilliseconds = _nextRetryDelayMilliseconds * _retryPolicy.BackoffMultiplier;
nextMilliseconds = Math.Min(nextMilliseconds, _retryPolicy.MaxBackoff.TotalMilliseconds);

return Convert.ToInt32(nextMilliseconds);
}
Expand Down
2 changes: 1 addition & 1 deletion test/FunctionalTests/Client/RetryTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ Task<DataMessage> UnaryFailure(DataMessage request, ServerCallContext context)
tcs.SetResult(new DataMessage());
}

[TestCase(0)]
[TestCase(1)]
[TestCase(20)]
public async Task Unary_AttemptsGreaterThanDefaultClientLimit_LimitedAttemptsMade(int hedgingDelay)
{
Expand Down
16 changes: 9 additions & 7 deletions test/Grpc.Net.Client.Tests/Retry/HedgingCallTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public async Task Dispose_ActiveCalls_CleansUpActiveCalls()
});
var serviceConfig = ServiceConfigHelpers.CreateHedgingServiceConfig(maxAttempts: 5, hedgingDelay: TimeSpan.FromMilliseconds(20));
var invoker = HttpClientCallInvokerFactory.Create(httpClient, serviceConfig: serviceConfig);
var hedgingCall = new HedgingCall<HelloRequest, HelloReply>(serviceConfig.MethodConfigs[0].HedgingPolicy!, invoker.Channel, ClientTestHelpers.ServiceMethod, new CallOptions());
var hedgingCall = new HedgingCall<HelloRequest, HelloReply>(CreateHedgingPolicy(serviceConfig.MethodConfigs[0].HedgingPolicy), invoker.Channel, ClientTestHelpers.ServiceMethod, new CallOptions());

// Act
hedgingCall.StartUnary(new HelloRequest { Name = "World" });
Expand All @@ -86,6 +86,8 @@ public async Task Dispose_ActiveCalls_CleansUpActiveCalls()
waitUntilFinishedTcs.SetResult(null);
}

private HedgingPolicyInfo CreateHedgingPolicy(HedgingPolicy? hedgingPolicy) => GrpcMethodInfo.CreateHedgingPolicy(hedgingPolicy!);

[Test]
public async Task ActiveCalls_FatalStatusCode_CleansUpActiveCalls()
{
Expand Down Expand Up @@ -120,7 +122,7 @@ public async Task ActiveCalls_FatalStatusCode_CleansUpActiveCalls()
});
var serviceConfig = ServiceConfigHelpers.CreateHedgingServiceConfig(maxAttempts: 5, hedgingDelay: TimeSpan.FromMilliseconds(20));
var invoker = HttpClientCallInvokerFactory.Create(httpClient, serviceConfig: serviceConfig);
var hedgingCall = new HedgingCall<HelloRequest, HelloReply>(serviceConfig.MethodConfigs[0].HedgingPolicy!, invoker.Channel, ClientTestHelpers.ServiceMethod, new CallOptions());
var hedgingCall = new HedgingCall<HelloRequest, HelloReply>(CreateHedgingPolicy(serviceConfig.MethodConfigs[0].HedgingPolicy), invoker.Channel, ClientTestHelpers.ServiceMethod, new CallOptions());

// Act
hedgingCall.StartUnary(new HelloRequest { Name = "World" });
Expand Down Expand Up @@ -183,7 +185,7 @@ public async Task ClientStreamWriteAsync_NoActiveCalls_WaitsForNextCall()
});
var serviceConfig = ServiceConfigHelpers.CreateHedgingServiceConfig(maxAttempts: 5, hedgingDelay: TimeSpan.FromMilliseconds(200));
var invoker = HttpClientCallInvokerFactory.Create(httpClient, serviceConfig: serviceConfig);
var hedgingCall = new HedgingCall<HelloRequest, HelloReply>(serviceConfig.MethodConfigs[0].HedgingPolicy!, invoker.Channel, ClientTestHelpers.GetServiceMethod(MethodType.ClientStreaming), new CallOptions());
var hedgingCall = new HedgingCall<HelloRequest, HelloReply>(CreateHedgingPolicy(serviceConfig.MethodConfigs[0].HedgingPolicy), invoker.Channel, ClientTestHelpers.GetServiceMethod(MethodType.ClientStreaming), new CallOptions());

// Act
hedgingCall.StartClientStreaming();
Expand Down Expand Up @@ -250,7 +252,7 @@ public async Task ResponseAsync_PushbackStop_SuccessAfterPushbackStop()
});
var serviceConfig = ServiceConfigHelpers.CreateHedgingServiceConfig(maxAttempts: 2);
var invoker = HttpClientCallInvokerFactory.Create(httpClient, serviceConfig: serviceConfig);
var hedgingCall = new HedgingCall<HelloRequest, HelloReply>(serviceConfig.MethodConfigs[0].HedgingPolicy!, invoker.Channel, ClientTestHelpers.ServiceMethod, new CallOptions());
var hedgingCall = new HedgingCall<HelloRequest, HelloReply>(CreateHedgingPolicy(serviceConfig.MethodConfigs[0].HedgingPolicy), invoker.Channel, ClientTestHelpers.ServiceMethod, new CallOptions());

// Act
hedgingCall.StartUnary(new HelloRequest { Name = "World" });
Expand Down Expand Up @@ -289,7 +291,7 @@ public async Task RetryThrottling_BecomesActiveDuringDelay_CancelFailure()
TokenRatio = 0.1
});
var invoker = HttpClientCallInvokerFactory.Create(httpClient, serviceConfig: serviceConfig);
var hedgingCall = new HedgingCall<HelloRequest, HelloReply>(serviceConfig.MethodConfigs[0].HedgingPolicy!, invoker.Channel, ClientTestHelpers.ServiceMethod, new CallOptions());
var hedgingCall = new HedgingCall<HelloRequest, HelloReply>(CreateHedgingPolicy(serviceConfig.MethodConfigs[0].HedgingPolicy), invoker.Channel, ClientTestHelpers.ServiceMethod, new CallOptions());

// Act
hedgingCall.StartUnary(new HelloRequest());
Expand Down Expand Up @@ -324,7 +326,7 @@ public async Task AsyncUnaryCall_CancellationDuringBackoff_CanceledStatus()
var cts = new CancellationTokenSource();
var serviceConfig = ServiceConfigHelpers.CreateHedgingServiceConfig(hedgingDelay: TimeSpan.FromSeconds(10));
var invoker = HttpClientCallInvokerFactory.Create(httpClient, serviceConfig: serviceConfig);
var hedgingCall = new HedgingCall<HelloRequest, HelloReply>(serviceConfig.MethodConfigs[0].HedgingPolicy!, invoker.Channel, ClientTestHelpers.ServiceMethod, new CallOptions(cancellationToken: cts.Token));
var hedgingCall = new HedgingCall<HelloRequest, HelloReply>(CreateHedgingPolicy(serviceConfig.MethodConfigs[0].HedgingPolicy), invoker.Channel, ClientTestHelpers.ServiceMethod, new CallOptions(cancellationToken: cts.Token));

// Act
hedgingCall.StartUnary(new HelloRequest());
Expand Down Expand Up @@ -353,7 +355,7 @@ public async Task AsyncUnaryCall_DisposeDuringBackoff_CanceledStatus()
});
var serviceConfig = ServiceConfigHelpers.CreateHedgingServiceConfig(hedgingDelay: TimeSpan.FromSeconds(10));
var invoker = HttpClientCallInvokerFactory.Create(httpClient, serviceConfig: serviceConfig);
var hedgingCall = new HedgingCall<HelloRequest, HelloReply>(serviceConfig.MethodConfigs[0].HedgingPolicy!, invoker.Channel, ClientTestHelpers.ServiceMethod, new CallOptions());
var hedgingCall = new HedgingCall<HelloRequest, HelloReply>(CreateHedgingPolicy(serviceConfig.MethodConfigs[0].HedgingPolicy), invoker.Channel, ClientTestHelpers.ServiceMethod, new CallOptions());

// Act
hedgingCall.StartUnary(new HelloRequest());
Expand Down
2 changes: 1 addition & 1 deletion test/Grpc.Net.Client.Tests/Retry/HedgingTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ namespace Grpc.Net.Client.Tests.Retry
[TestFixture]
public class HedgingTests
{
[TestCase(1)]
[TestCase(2)]
[TestCase(10)]
[TestCase(100)]
public async Task AsyncUnaryCall_OneAttempt_Success(int maxAttempts)
Expand Down
Loading

0 comments on commit 5df6bf2

Please sign in to comment.