Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken REST host fallbacks #1278

Merged
merged 57 commits into from
Apr 19, 2024
Merged
Show file tree
Hide file tree
Changes from 56 commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
a337a44
Updated exception message for max no. of retries
sacOO7 Mar 26, 2024
d5b2bf2
Refactored implementation for fullresthost and fullrealtimehost
sacOO7 Mar 27, 2024
76191ab
refactored implementation for generating fallback hosts
sacOO7 Mar 27, 2024
dda0a8e
Refactored httpMaxRetryCount to be set based on external http options
sacOO7 Mar 27, 2024
4610d2c
removed unnecessary check for canretry for fallback hosts
sacOO7 Mar 27, 2024
9159f53
refactored implementation for max no. of retries
sacOO7 Mar 27, 2024
5231786
Disabled treating warning as errors
sacOO7 Mar 27, 2024
55fe1ea
Revert "Disabled treating warning as errors"
sacOO7 Mar 27, 2024
77cf470
suppressed newtonsoft depecated package warning for netstandard
sacOO7 Mar 27, 2024
1b0c411
disabled warnings as errors for netframework
sacOO7 Mar 27, 2024
0f058c7
Disabled warning for specified error code
sacOO7 Mar 27, 2024
91044f4
updated netstandard csproj to ignore warnings as errors with end flag
sacOO7 Mar 28, 2024
7f77374
removed treat warnings as errors from netstandard project
sacOO7 Mar 28, 2024
84fbeae
set treatwarningsaserrors to false in netstandard project
sacOO7 Mar 28, 2024
0c52be5
disabled warnings as errors in test netframework project
sacOO7 Mar 28, 2024
f82f485
updated paket dependencies to fix failing logging CI build issues
sacOO7 Mar 28, 2024
dcabd58
deprecated paket.dependencies to resolve logging issue
sacOO7 Mar 28, 2024
f86c357
Added msbuild structuredlogger to paket dependencies
sacOO7 Mar 28, 2024
208e0fa
Added msbuild structuredlogger as an explicit dependency
sacOO7 Mar 28, 2024
be66e8a
updated paket.dependencies to include structuredlogger
sacOO7 Mar 28, 2024
d07e389
included necessary dependencies for structured logger
sacOO7 Mar 28, 2024
0079171
using latest version of structuredlogger in the project instead
sacOO7 Mar 28, 2024
053b854
Deprecated required version for structuredlogger
sacOO7 Mar 28, 2024
beaf4f1
updated CI net6 build sdk to latest
sacOO7 Mar 28, 2024
71be70b
disabled msbuild structuredlogger from build.fs file
sacOO7 Mar 28, 2024
0102d40
Fixed build.fs liniting issue by running dotnet fantomas
sacOO7 Mar 28, 2024
f13f6f5
Added flag to disable in proc node
sacOO7 Mar 28, 2024
1201077
Revert "Added flag to disable in proc node"
sacOO7 Mar 28, 2024
39ddae2
deprecated build framework dependencies
sacOO7 Mar 28, 2024
ee73032
Disabled big log for dotnet retry tests F# script
sacOO7 Mar 28, 2024
a173f68
Revert "Disabled big log for dotnet retry tests F# script"
sacOO7 Mar 28, 2024
2183374
Revert "updated CI net6 build sdk to latest"
sacOO7 Mar 28, 2024
c552f05
deprecated all of the dependencies for F# build script in accordance …
sacOO7 Mar 28, 2024
f90dd66
Added extra dependency for structuredlogger
sacOO7 Mar 28, 2024
e4f6d09
set build project target framework to 7
sacOO7 Mar 28, 2024
bbb0183
Revert "Added extra dependency for structuredlogger"
sacOO7 Mar 28, 2024
dd508b5
updated global.json to use dotnet 7 instead of 6
sacOO7 Mar 28, 2024
926615d
Reapply "Added extra dependency for structuredlogger"
sacOO7 Mar 28, 2024
5c803a5
updated dotnet to latest version
sacOO7 Mar 28, 2024
00674c4
set dotnet global version to 7.0.305
sacOO7 Apr 4, 2024
acef789
Refactored AblyHttpClient, fixed attempt counter
sacOO7 Apr 5, 2024
093760f
Fixed failing tests for rest fallbacks, added wrap function for
sacOO7 Apr 5, 2024
54a5a46
refactored fallbacks implementation, wrapped with request id properly
sacOO7 Apr 5, 2024
c6884f4
reverted all structured logger related changes
sacOO7 Apr 11, 2024
9bcbe36
updated global.json to use exact mentioned version of dotnet
sacOO7 Apr 11, 2024
4f54b5d
Downloaded missing dotnet 7 for netframework and xamarin build
sacOO7 Apr 11, 2024
29117ae
updated target framework for build script fsproj to net 7.0
sacOO7 Apr 11, 2024
5951804
Downgraded dotnet 7 build dependency for F# build project
sacOO7 Apr 11, 2024
3f59296
Using latest version of dotnet 7 for building the project
sacOO7 Apr 11, 2024
2fc51da
Fixed failing test for authsandboxspec
sacOO7 Apr 18, 2024
4c2ab86
Ignored flaky test helper test for checking timeout elapsed
sacOO7 Apr 18, 2024
f4bcd48
Merge branch 'main' into fix/broken-fallbacks
sacOO7 Apr 18, 2024
3054ce5
Removed unnecessary isdefault host check from tests
sacOO7 Apr 18, 2024
c2d5739
Added test to check for provided custom fallback hosts
sacOO7 Apr 19, 2024
e044c22
Refactored code that checks for default hosts when retrying fallbacks
sacOO7 Apr 19, 2024
9a80c7e
Removed unnecessary test for fallbacks with default host
sacOO7 Apr 19, 2024
df35dbd
Refactored AblyHttpClient code as per review comments
sacOO7 Apr 19, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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>
sacOO7 marked this conversation as resolved.
Show resolved Hide resolved
<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; // One for the first request
VeskeR marked this conversation as resolved.
Show resolved Hide resolved
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);
VeskeR marked this conversation as resolved.
Show resolved Hide resolved

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
Loading
Loading