Skip to content

Commit d9d658b

Browse files
Jon Skeetjskeet
authored andcommitted
feat: Make StreamingPull fail after 100 consecutive failures
With a max backoff of 30 seconds (which we reach pretty quickly) this means we'll only actually fail after about 45 minutes, so it does allow quite a lot of time for recovery.
1 parent 335abae commit d9d658b

2 files changed

Lines changed: 33 additions & 4 deletions

File tree

apis/Google.Cloud.PubSub.V1/Google.Cloud.PubSub.V1.Tests/SubscriberClientTest.cs

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
using System;
2424
using System.Collections.Generic;
2525
using System.Linq;
26+
using System.Text.RegularExpressions;
2627
using System.Threading;
2728
using System.Threading.Tasks;
2829
using Xunit;
@@ -1784,7 +1785,7 @@ public void StreamingPullRetry_NonRetriableException()
17841785
}
17851786

17861787
[Fact]
1787-
public void StreamingPullRetry_InternalErrorRetriesForever()
1788+
public void StreamingPullRetry_InternalErrorContinuesRetrying()
17881789
{
17891790
// A regular internal failure that's not due to an auth error.
17901791
var exception = new RpcException(new Status(StatusCode.Internal, "Bang"));
@@ -1801,6 +1802,24 @@ public void StreamingPullRetry_InternalErrorRetriesForever()
18011802
});
18021803
}
18031804

1805+
[Fact]
1806+
public void StreamingPullRetry_RetriableErrorEventuallyFails()
1807+
{
1808+
// A regular internal failure that's not due to an auth error.
1809+
var exception = new RpcException(new Status(StatusCode.Internal, "Bang"));
1810+
1811+
// When we've reached a limit of the number of exceptions we're happy to retry, we'll eventually fail.
1812+
// (This will take a long time, with all the backoffs involved...)
1813+
using var fake = Fake.Create(CreateBadMoveNextSequence(TimeSpan.FromSeconds(1), exception, 100, includeTrailing: true));
1814+
1815+
fake.Scheduler.Run(async () =>
1816+
{
1817+
var subscriberTask = fake.Subscriber.StartAsync((msg, ct) => throw new Exception("No messages should be provided"));
1818+
var subscriberEx = await Assert.ThrowsAsync<RpcException>(() => subscriberTask);
1819+
Assert.Equal(exception.Status, subscriberEx.Status);
1820+
});
1821+
}
1822+
18041823
/// <summary>
18051824
/// If the streaming pull call fails in MoveNext after a short time (e.g. 10 seconds)
18061825
/// we should retry with backoff.

apis/Google.Cloud.PubSub.V1/Google.Cloud.PubSub.V1/SubscriberClientImpl.SingleChannel.cs

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1122,7 +1122,11 @@ private class RetryState
11221122
private const string GrpcCoreAuthExceptionPrefix = "Getting metadata from plugin failed with error: Exception occurred in metadata credentials plugin. ";
11231123
private const int MaxAuthExceptionsBeforeFailing = 4;
11241124

1125-
private const int ExceptionLimit = 100;
1125+
/// <summary>
1126+
/// We fail after this many consecutive failures, regardless of what the failure was.
1127+
/// With the backoffs involved, this will be after a pretty significant amount of time anyway.
1128+
/// </summary>
1129+
private const int ConcurrentFailureLimit = 100;
11261130

11271131
private readonly IClock _clock;
11281132
private readonly ILogger _logger;
@@ -1160,9 +1164,13 @@ internal bool RecordFailureAndCheckForRetry(Exception exception)
11601164
{
11611165
return false;
11621166
}
1163-
if (_exceptions.Count < ExceptionLimit)
1167+
1168+
_exceptions.Add(rpcEx);
1169+
1170+
// If we've reached our limit, fail regardless.
1171+
if (_exceptions.Count == ConcurrentFailureLimit)
11641172
{
1165-
_exceptions.Add(rpcEx);
1173+
return false;
11661174
}
11671175

11681176
var code = rpcEx.StatusCode;
@@ -1183,6 +1191,8 @@ internal bool RecordFailureAndCheckForRetry(Exception exception)
11831191
}
11841192

11851193
// If the exception was a failure due to auth, and we've seen some before, don't retry.
1194+
// The auth-related exceptions don't need to be consecutive: if we have transient auth related
1195+
// problems, then non-auth problems, then auth related problems again, we're definitely in a bad situation.
11861196
if (IsAuthException(rpcEx))
11871197
{
11881198
var count = _exceptions.Count(IsAuthException);

0 commit comments

Comments
 (0)