Skip to content

Commit

Permalink
Merge pull request #1278 from ably/fix/broken-fallbacks
Browse files Browse the repository at this point in the history
Fix broken REST host fallbacks
  • Loading branch information
sacOO7 committed Apr 19, 2024
2 parents e6e7537 + df35dbd commit 6a4b476
Show file tree
Hide file tree
Showing 14 changed files with 103 additions and 82 deletions.
1 change: 1 addition & 0 deletions .github/workflows/run-tests-macos-xamarin.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ jobs:
with:
dotnet-version: |
3.1.x
7.0.408
- name: Download dotnet build-script tools
run: dotnet tool restore
Expand Down
6 changes: 6 additions & 0 deletions .github/workflows/run-tests-windows-netframework.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ jobs:
with:
submodules: 'recursive'

- name: Download dotnet framework
uses: actions/setup-dotnet@v3
with:
dotnet-version: |
7.0.408
- name: Download dotnet build-script tools
run: dotnet tool restore

Expand Down
4 changes: 2 additions & 2 deletions build-script/build-script.fsproj
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net6.0</TargetFramework>
<TargetFramework>net7.0</TargetFramework>
<RootNamespace>build_script</RootNamespace>
<WarnOn>3390;$(WarnOn)</WarnOn>
<NoWarn>0020</NoWarn>
Expand All @@ -12,4 +12,4 @@
<Compile Include="build.fs" />
</ItemGroup>
<Import Project=".paket\Paket.Restore.targets" />
</Project>
</Project>
2 changes: 1 addition & 1 deletion build-script/paket.references
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@ Microsoft.Build.Tasks.Core
Fake.Core.Target
Fake.DotNet.AssemblyInfoFile
Fake.DotNet.Cli
Fake.DotNet.Testing.XUnit2
Fake.DotNet.Testing.XUnit2
2 changes: 1 addition & 1 deletion src/IO.Ably.NETFramework/IO.Ably.NETFramework.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -108,4 +108,4 @@
</PropertyGroup>
<Error Condition="!Exists('..\packages\Microsoft.CodeQuality.Analyzers.2.9.6\build\Microsoft.CodeQuality.Analyzers.props')" Text="$([System.String]::Format('$(ErrorText)', '..\packages\Microsoft.CodeQuality.Analyzers.2.9.6\build\Microsoft.CodeQuality.Analyzers.props'))" />
</Target>
</Project>
</Project>
53 changes: 21 additions & 32 deletions src/IO.Ably.Shared/ClientOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,6 @@ public string[] FallbackHosts

internal bool IsProductionEnvironment => Environment.IsEmpty() || Environment.Equals("production", StringComparison.OrdinalIgnoreCase);

internal bool IsDefaultRestHost => FullRestHost() == Defaults.RestHost;

internal bool IsDefaultRealtimeHost => FullRealtimeHost() == Defaults.RealtimeHost;

internal bool IsDefaultPort => Tls ? TlsPort == Defaults.TlsPort : Port == Defaults.Port;

/// <summary>
Expand All @@ -159,20 +155,17 @@ public string[] FallbackHosts
/// <returns>RestHost.</returns>
public string FullRestHost()
{
var restHost = _restHost;
if (restHost.IsEmpty())
if (_restHost.IsNotEmpty())
{
restHost = Defaults.RestHost;
return _restHost;
}

if (restHost == Defaults.RestHost)
if (!IsProductionEnvironment)
{
return IsProductionEnvironment
? restHost
: $"{Environment}-{restHost}";
return $"{Environment}-{Defaults.RestHost}";
}

return restHost;
return Defaults.RestHost;
}

/// <summary>
Expand All @@ -181,27 +174,26 @@ public string FullRestHost()
/// <returns>RealtimeHost.</returns>
public string FullRealtimeHost()
{
var realtimeHost = _realtimeHost;
if (realtimeHost.IsEmpty())
if (_realtimeHost.IsNotEmpty())
{
if (_restHost.IsNotEmpty())
{
Logger.Warning(
$@"restHost is set to {_restHost} but realtimeHost is not set,
so setting realtimeHost to {_restHost} too. If this is not what you want,
please set realtimeHost explicitly.");
return _restHost;
}
return _realtimeHost;
}

realtimeHost = Defaults.RealtimeHost;
if (_restHost.IsNotEmpty())
{
Logger.Warning(
$@"restHost is set to {_restHost} but realtimeHost is not set,
so setting realtimeHost to {_restHost} too. If this is not what you want,
please set realtimeHost explicitly.");
return _restHost;
}

if (realtimeHost == Defaults.RealtimeHost)
if (!IsProductionEnvironment)
{
return IsProductionEnvironment ? realtimeHost : $"{Environment}{'-'}{realtimeHost}";
return $"{Environment}-{Defaults.RealtimeHost}";
}

return realtimeHost;
return Defaults.RealtimeHost;
}

/// <summary>
Expand All @@ -220,7 +212,7 @@ public string[] GetFallbackHosts()
throw new AblyException(new ErrorInfo(msg, ErrorCodes.BadRequest));
}

if (Port != Defaults.Port || TlsPort != Defaults.TlsPort)
if (!IsDefaultPort)
{
const string msg = "fallbackHostsUseDefault cannot be set when port or tlsPort are set";
throw new AblyException(new ErrorInfo(msg, ErrorCodes.BadRequest));
Expand All @@ -230,15 +222,12 @@ public string[] GetFallbackHosts()
{
Logger.Warning("Deprecated fallbackHostsUseDefault : There is no longer a need to set this when the environment option is also set since the library will now generate the correct fallback hosts using the environment option.");
}
else
{
Logger.Warning("Deprecated fallbackHostsUseDefault : fallbackHosts: Ably.Defaults.FALLBACK_HOSTS");
}

Logger.Warning("Deprecated fallbackHostsUseDefault : fallbackHosts: Ably.Defaults.FALLBACK_HOSTS");
return Defaults.FallbackHosts;
}

if (_fallbackHosts is null && _restHost is null && _realtimeHost is null && IsDefaultPort)
if (_fallbackHosts is null && _restHost.IsEmpty() && _realtimeHost.IsEmpty() && IsDefaultPort)
{
return IsProductionEnvironment
? Defaults.FallbackHosts
Expand Down
26 changes: 12 additions & 14 deletions src/IO.Ably.Shared/Http/AblyHttpClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,6 @@ internal string RealtimeConnectedFallbackHost

private DateTimeOffset? FallbackHostUsedFrom { get; set; }

private bool CanRetry => Options.IsDefaultHost || Options.FallbackHostsUseDefault;

internal void SetPreferredHost(string currentHost)
{
// If we are retrying the default host we need to clear the preferred one
Expand Down Expand Up @@ -90,7 +88,7 @@ public async Task<AblyResponse> Execute(AblyRequest request)
int currentTry = 0;
var startTime = Now();

var numberOfRetries = Options.HttpMaxRetryCount; // One for the first request
var maxNumberOfRetries = Options.HttpMaxRetryCount;
var host = GetHost();

request.Headers.TryGetValue("request_id", out var requestId);
Expand All @@ -115,22 +113,20 @@ public async Task<AblyResponse> Execute(AblyRequest request)
break;
}

if (CanRetry && response.CanRetry)
if (response.CanRetry)
{
currentTry++;

Logger.Warning(WrapWithRequestId("Failed response. " + response.GetFailedMessage() + ". Retrying..."));
var (success, newHost) = HandleHostChangeForRetryableFailure();
var (success, newHost) = HandleHostChangeForRetryableFailure(currentTry);
if (success)
{
Logger.Debug(WrapWithRequestId($"Retrying using host: {newHost}"));

host = newHost;
continue;
}
}

// We only return the request if there is no exception
// We only return the response if there is no exception
if (request.NoExceptionOnHttpError && response.Exception == null)
{
return response.AblyResponse;
Expand All @@ -140,19 +136,21 @@ public async Task<AblyResponse> Execute(AblyRequest request)
// in that case we need to create a new AblyException
throw response.Exception ?? AblyException.FromResponse(response.AblyResponse);
}
catch (AblyException)
catch (AblyException ex)
{
throw;
var errInfo = ex.ErrorInfo;
errInfo.Message = WrapWithRequestId("Error executing request. " + errInfo.Message);
throw new AblyException(errInfo, ex);
}
catch (Exception ex)
{
// TODO: Sentry logging here
throw new AblyException(new ErrorInfo(WrapWithRequestId("Error executing request. " + ex.Message), ErrorCodes.InternalError), ex);
}
}
while (currentTry < numberOfRetries);
while (currentTry <= maxNumberOfRetries); // 1 primary host and remaining fallback hosts

throw new AblyException(new ErrorInfo(WrapWithRequestId("Error executing request"), ErrorCodes.InternalError));
throw new AblyException(new ErrorInfo(WrapWithRequestId("Error executing request, exceeded max no. of retries"), ErrorCodes.InternalError));

List<string> GetFallbackHosts()
{
Expand Down Expand Up @@ -251,15 +249,15 @@ async Task<HttpResponseWrapper> MakeRequest(string requestHost)
}
}

(bool success, string host) HandleHostChangeForRetryableFailure()
(bool success, string host) HandleHostChangeForRetryableFailure(int attempt)
{
if (fallbackHosts.Count == 0)
{
Logger.Debug(WrapWithRequestId("No more hosts left to retry. Cannot assign a new fallback host."));
return (false, null);
}

bool isFirstTryForRequest = currentTry == 1;
bool isFirstTryForRequest = attempt == 1;

// If there is a Preferred fallback host already set
// and it failed we should try the RealtimeConnected fallback host first
Expand Down
5 changes: 1 addition & 4 deletions src/IO.Ably.Shared/Http/AblyHttpOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@ internal class AblyHttpOptions

internal TimeSpan FallbackRetryTimeOut { get; set; }

public bool IsDefaultHost { get; set; } = true;

internal string[] FallbackHosts { get; set; }

internal bool FallbackHostsUseDefault { get; set; }
Expand Down Expand Up @@ -64,12 +62,11 @@ public AblyHttpOptions(ClientOptions options)
IsSecure = options.Tls;
Port = options.Tls ? options.TlsPort : options.Port;
Host = options.FullRestHost();
IsDefaultHost = options.IsDefaultRestHost;
DisconnectedRetryTimeout = options.DisconnectedRetryTimeout;
SuspendedRetryTimeout = options.SuspendedRetryTimeout;
HttpOpenTimeout = options.HttpOpenTimeout;
HttpRequestTimeout = options.HttpRequestTimeout;
HttpMaxRetryCount = options.IsDefaultRestHost ? options.HttpMaxRetryCount : 1;
HttpMaxRetryCount = options.HttpMaxRetryCount;
HttpMaxRetryDuration = options.HttpMaxRetryDuration;
FallbackRetryTimeOut = options.FallbackRetryTimeout;
FallbackHosts = options.GetFallbackHosts();
Expand Down
9 changes: 1 addition & 8 deletions src/IO.Ably.Shared/Realtime/AttemptsHelpers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,7 @@ internal static class AttemptsHelpers
{
public static async Task<bool> CanFallback(this AblyRest restClient, ErrorInfo error)
{
return IsDefaultHost() &&
error != null && error.IsRetryableStatusCode() &&
await restClient.CanConnectToAbly();

bool IsDefaultHost()
{
return restClient.Options.IsDefaultRealtimeHost;
}
return error != null && error.IsRetryableStatusCode() && await restClient.CanConnectToAbly();
}

public static bool ShouldSuspend(this RealtimeState state, Func<DateTimeOffset> now = null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,4 +153,4 @@
</ItemGroup>
<Import Project="..\IO.Ably.Tests.Shared\IO.Ably.Tests.Shared.projitems" Label="Shared" />
<Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
</Project>
</Project>
7 changes: 7 additions & 0 deletions src/IO.Ably.Tests.Shared/Infrastructure/TestHelpers.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using IO.Ably.Tests.Infrastructure;
using Xunit;
Expand All @@ -7,6 +9,11 @@ namespace IO.Ably.Tests
{
internal static class TestHelpers
{
public static bool IsSubsetOf<T>(this IEnumerable<T> a, IEnumerable<T> b)
{
return !a.Except(b).Any();
}

public static void AssertContainsParameter(this AblyRequest request, string key, string value)
{
Assert.True(
Expand Down
11 changes: 1 addition & 10 deletions src/IO.Ably.Tests.Shared/Realtime/ConnectionAttemptsInfoSpecs.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,6 @@ public void ShouldSuspend_WhenFirstAttemptEqualOrGreaterThanConnectionStateTtl_S
state.ShouldSuspend(now.ValueFn).Should().BeTrue("When time is greater than"); // >
}

[Fact]
public async Task CanAttemptFallback_ShouldBeFalseWithNonDefaultHost()
{
var client = GetRealtime(opts => opts.RealtimeHost = "test.test.com");

var result = await client.RestClient.CanFallback(null);
result.Should().BeFalse();
}

[Theory]
[InlineData(500)]
[InlineData(501)]
Expand All @@ -84,7 +75,7 @@ public async Task CanAttemptFallback_WithDefaultHostAndAppropriateError_ShouldBe
public async Task CanAttemptFallback_WhenInternetCheckFails_ShouldBeFalse()
{
var client = GetRealtime(internetCheckOk: false);
var result = await client.RestClient.CanFallback(null);
var result = await client.RestClient.CanFallback(ErrorInfo.ReasonUnknown);
result.Should().BeFalse();
}

Expand Down
2 changes: 0 additions & 2 deletions src/IO.Ably.Tests.Shared/Rest/ChannelSandboxSpecs.cs
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,6 @@ public async Task IdempotentPublishing_SimulateErrorAndRetry(Protocol protocol)
{
// setting IsDefaultHost and raising a TaskCanceledException
// will cause the request to retry
client.HttpClient.Options.IsDefaultHost = true;
throw new TaskCanceledException("faked exception to cause retry");
}
Expand Down Expand Up @@ -301,7 +300,6 @@ public async Task IdempotentPublishing_WithClientSpecificMessage_ShouldRetry(Pro
{
// setting IsDefaultHost and raising a TaskCanceledException
// will cause the request to retry
client.HttpClient.Options.IsDefaultHost = true;
throw new TaskCanceledException("faked exception to cause retry");
}
Expand Down

0 comments on commit 6a4b476

Please sign in to comment.