From bf18a3abe6ca32173d9ab2c6f32e284ac630a8c3 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Thu, 1 Aug 2024 09:01:15 +1200 Subject: [PATCH] Ensure cancelation works properly in PollingNetworkStatusListener (#3506) --- .../Internal/PollingNetworkStatusListener.cs | 21 +++++++---- src/Sentry/Internal/TcpPing.cs | 12 +++++-- .../Internals/Http/CachingTransportTests.cs | 2 +- .../PollingNetworkStatusListenerTests.cs | 36 ++++++++++++++++--- 4 files changed, 57 insertions(+), 14 deletions(-) diff --git a/src/Sentry/Internal/PollingNetworkStatusListener.cs b/src/Sentry/Internal/PollingNetworkStatusListener.cs index f5442038a6..f3f2a44246 100644 --- a/src/Sentry/Internal/PollingNetworkStatusListener.cs +++ b/src/Sentry/Internal/PollingNetworkStatusListener.cs @@ -54,16 +54,23 @@ public async Task WaitForNetworkOnlineAsync(CancellationToken cancellationToken { while (!cancellationToken.IsCancellationRequested) { - await Task.Delay(_delayInMilliseconds, cancellationToken).ConfigureAwait(false); - var checkResult = await Ping.IsAvailableAsync().ConfigureAwait(false); - if (checkResult) + try { - Online = true; - return; + await Task.Delay(_delayInMilliseconds, cancellationToken).ConfigureAwait(false); + var checkResult = await Ping.IsAvailableAsync(cancellationToken).ConfigureAwait(false); + if (checkResult) + { + Online = true; + return; + } + if (_delayInMilliseconds < _maxDelayInMilliseconds) + { + _delayInMilliseconds = _backoffFunction(_delayInMilliseconds); + } } - if (_delayInMilliseconds < _maxDelayInMilliseconds) + catch (OperationCanceledException) { - _delayInMilliseconds = _backoffFunction(_delayInMilliseconds); + break; } } } diff --git a/src/Sentry/Internal/TcpPing.cs b/src/Sentry/Internal/TcpPing.cs index b923e924d7..7a922c96e6 100644 --- a/src/Sentry/Internal/TcpPing.cs +++ b/src/Sentry/Internal/TcpPing.cs @@ -2,21 +2,29 @@ namespace Sentry.Internal; internal interface IPing { - Task IsAvailableAsync(); + Task IsAvailableAsync(CancellationToken cancellationToken); } internal class TcpPing(string hostToCheck, int portToCheck = 443) : IPing { private readonly Ping _ping = new(); - public async Task IsAvailableAsync() + public async Task IsAvailableAsync(CancellationToken cancellationToken) { try { using var tcpClient = new TcpClient(); +#if NET5_0_OR_GREATER + await tcpClient.ConnectAsync(hostToCheck, portToCheck, cancellationToken).ConfigureAwait(false); +#else await tcpClient.ConnectAsync(hostToCheck, portToCheck).ConfigureAwait(false); +#endif return true; } + catch (OperationCanceledException) + { + throw; + } catch (Exception) { return false; diff --git a/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs b/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs index ede49e020d..037e0ca136 100644 --- a/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs +++ b/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs @@ -548,7 +548,7 @@ public async Task TestNetworkException(Exception exception) // Arrange - network unavailable using var cacheDirectory = new TempDirectory(_fileSystem); var pingHost = Substitute.For(); - pingHost.IsAvailableAsync().Returns(Task.FromResult(true)); + pingHost.IsAvailableAsync(Arg.Any()).Returns(Task.FromResult(true)); var options = new SentryOptions { Dsn = ValidDsn, diff --git a/test/Sentry.Tests/Internals/PollingNetworkStatusListenerTests.cs b/test/Sentry.Tests/Internals/PollingNetworkStatusListenerTests.cs index d125324b06..30417285dc 100644 --- a/test/Sentry.Tests/Internals/PollingNetworkStatusListenerTests.cs +++ b/test/Sentry.Tests/Internals/PollingNetworkStatusListenerTests.cs @@ -9,7 +9,7 @@ public async Task HostAvailable_CheckOnlyRunsOnce() var initialDelay = 100; var pingHost = Substitute.For(); pingHost - .IsAvailableAsync() + .IsAvailableAsync(Arg.Any()) .Returns(Task.FromResult(true)); var pollingListener = new PollingNetworkStatusListener(pingHost, initialDelay); @@ -23,7 +23,7 @@ public async Task HostAvailable_CheckOnlyRunsOnce() // Assert completedTask.Should().Be(waitForNetwork); pollingListener.Online.Should().Be(true); - await pingHost.Received(1).IsAvailableAsync(); + await pingHost.Received(1).IsAvailableAsync(Arg.Any()); } [Fact] @@ -33,7 +33,7 @@ public async Task HostUnavailable_ShouldIncreaseDelay() var initialDelay = 100; // set initial delay to ease the testing var pingHost = Substitute.For(); pingHost - .IsAvailableAsync() + .IsAvailableAsync(Arg.Any()) .Returns(Task.FromResult(false)); var pollingListener = new PollingNetworkStatusListener(pingHost, initialDelay); @@ -47,7 +47,35 @@ public async Task HostUnavailable_ShouldIncreaseDelay() // Assert completedTask.Should().Be(timeout); pollingListener.Online.Should().Be(false); - await pingHost.Received().IsAvailableAsync(); + await pingHost.Received().IsAvailableAsync(Arg.Any()); pollingListener._delayInMilliseconds.Should().BeGreaterThan(initialDelay); } + + [Fact] + public async Task OperationCancelled_ShouldExitGracefully() + { + // Arrange + const int initialDelay = 10_000; + var pingHost = Substitute.For(); + pingHost + .IsAvailableAsync(Arg.Any()) + .Returns(Task.FromResult(false)); + + var pollingListener = new PollingNetworkStatusListener(pingHost, initialDelay) + { + Online = false + }; + var cts = new CancellationTokenSource(); + + // Act + var waitForNetwork = pollingListener.WaitForNetworkOnlineAsync(cts.Token); + var timeout = Task.Delay(2000); + cts.CancelAfter(100); + var completedTask = await Task.WhenAny(waitForNetwork, timeout); + + // Assert + completedTask.Should().Be(waitForNetwork); + pollingListener.Online.Should().Be(false); + await completedTask; // Throws exception if the task is faulted + } }