diff --git a/CHANGELOG.md b/CHANGELOG.md index 1bceb59b3f..9296f87fec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ - Workaround `System.Text.Json` issue with Unity IL2CPP. ([#1583](https://github.com/getsentry/sentry-dotnet/pull/1583)) - Demystify stack traces for exceptions that fire in a `BeforeSend` callback. ([#1587](https://github.com/getsentry/sentry-dotnet/pull/1587)) +- Fix a minor issue in the caching transport related to recovery of files from previous session. ([#1617](https://github.com/getsentry/sentry-dotnet/pull/1617)) ## 3.16.0 diff --git a/src/Sentry/Internal/Http/CachingTransport.cs b/src/Sentry/Internal/Http/CachingTransport.cs index bf9d4a6b9e..9dfaf95d18 100644 --- a/src/Sentry/Internal/Http/CachingTransport.cs +++ b/src/Sentry/Internal/Http/CachingTransport.cs @@ -41,10 +41,10 @@ internal class CachingTransport : ITransport, IAsyncDisposable, IDisposable // Inner transport exposed internally primarily for testing internal ITransport InnerTransport => _innerTransport; - public static CachingTransport Create(ITransport innerTransport, SentryOptions options) + public static CachingTransport Create(ITransport innerTransport, SentryOptions options, bool startWorker = true) { var transport = new CachingTransport(innerTransport, options); - transport.Initialize(); + transport.Initialize(startWorker); return transport; } @@ -64,38 +64,21 @@ private CachingTransport(ITransport innerTransport, SentryOptions options) _processingDirectoryPath = Path.Combine(_isolatedCacheDirectoryPath, "__processing"); } - private void Initialize() + private void Initialize(bool startWorker) { + // Restore any abandoned files from a previous session + MoveUnprocessedFilesBackToCache(); + + // Ensure directories exist Directory.CreateDirectory(_isolatedCacheDirectoryPath); Directory.CreateDirectory(_processingDirectoryPath); - _worker = Task.Run(CachedTransportBackgroundTaskAsync); + // Start a worker, if one is needed + _worker = startWorker ? Task.Run(CachedTransportBackgroundTaskAsync) : Task.CompletedTask; } private async Task CachedTransportBackgroundTaskAsync() { - try - { - // Processing directory may already contain some files left from previous session - // if the worker has been terminated unexpectedly. - // Move everything from that directory back to cache directory. - if (Directory.Exists(_processingDirectoryPath)) - { - foreach (var filePath in Directory.EnumerateFiles(_processingDirectoryPath)) - { - var destinationPath = Path.Combine(_isolatedCacheDirectoryPath, Path.GetFileName(filePath)); - _options.LogDebug("Moving unprocessed file back to cache: {0} to {1}.", - filePath, destinationPath); - - File.Move(filePath, destinationPath); - } - } - } - catch (Exception e) - { - _options.LogError("Failed to move unprocessed files back to cache.", e); - } - while (!_workerCts.IsCancellationRequested) { try @@ -130,6 +113,34 @@ private async Task CachedTransportBackgroundTaskAsync() _options.LogDebug("Background worker of CachingTransport has shutdown."); } + private void MoveUnprocessedFilesBackToCache() + { + // Processing directory may already contain some files left from previous session + // if the cache was working when the process terminated unexpectedly. + // Move everything from that directory back to cache directory. + + if (!Directory.Exists(_processingDirectoryPath)) + { + // nothing to do + return; + } + + foreach (var filePath in Directory.EnumerateFiles(_processingDirectoryPath)) + { + try + { + var destinationPath = Path.Combine(_isolatedCacheDirectoryPath, Path.GetFileName(filePath)); + _options.LogDebug("Moving unprocessed file back to cache: {0} to {1}.", + filePath, destinationPath); + File.Move(filePath, destinationPath); + } + catch (Exception e) + { + _options.LogError("Failed to move unprocessed file back to cache: {0}", e, filePath); + } + } + } + private void EnsureFreeSpaceInCache() { // Trim files, leaving only (X - 1) of the newest ones. @@ -206,6 +217,13 @@ await using (stream.ConfigureAwait(false)) // Let the worker catch, log, wait a bit and retry. throw; } + + if (ex.Source == "FakeFailingTransport") + { + // HACK: Deliberately sent from unit tests to avoid deleting the file from processing + return; + } + LogFailureWithDiscard(file, ex); } } diff --git a/test/Sentry.Testing/FakeFailingTransport.cs b/test/Sentry.Testing/FakeFailingTransport.cs index 7e4f6851ba..1c1099e2da 100644 --- a/test/Sentry.Testing/FakeFailingTransport.cs +++ b/test/Sentry.Testing/FakeFailingTransport.cs @@ -6,6 +6,9 @@ internal class FakeFailingTransport : ITransport Envelope envelope, CancellationToken cancellationToken = default) { - throw new Exception("Expected transport failure has occured."); + throw new Exception("Expected transport failure has occured.") + { + Source = nameof(FakeFailingTransport) + }; } } diff --git a/test/Sentry.Testing/Waiter.cs b/test/Sentry.Testing/Waiter.cs new file mode 100644 index 0000000000..a826399ca1 --- /dev/null +++ b/test/Sentry.Testing/Waiter.cs @@ -0,0 +1,24 @@ +namespace Sentry.Testing; + +public sealed class Waiter : IDisposable +{ + private readonly TaskCompletionSource _taskCompletionSource = new(); + private readonly CancellationTokenSource _cancellationTokenSource = new(); + + public Waiter(Action> callback) + { + _cancellationTokenSource.Token.Register(() => _taskCompletionSource.SetCanceled()); + callback.Invoke((_, _) => _taskCompletionSource.SetResult(null)); + } + + public async Task WaitAsync(TimeSpan timeout) + { + _cancellationTokenSource.CancelAfter(timeout); + await _taskCompletionSource.Task; + } + + public void Dispose() + { + _cancellationTokenSource.Dispose(); + } +} diff --git a/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs b/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs index 4267802fd8..a09911bc30 100644 --- a/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs +++ b/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs @@ -15,7 +15,7 @@ public CachingTransportTests(ITestOutputHelper testOutputHelper) _logger = new TestOutputDiagnosticLogger(testOutputHelper); } - [Fact(Timeout = 7000)] + [Fact] public async Task WithAttachment() { // Arrange @@ -40,7 +40,8 @@ public async Task WithAttachment() exception = readStreamException; } }))); - await using var transport = CachingTransport.Create(innerTransport, options); + + await using var transport = CachingTransport.Create(innerTransport, options, startWorker: false); var tempFile = Path.GetTempFileName(); @@ -51,7 +52,7 @@ public async Task WithAttachment() // Act await transport.SendEnvelopeAsync(envelope); - await WaitForDirectoryToBecomeEmptyAsync(cacheDirectory.Path); + await transport.FlushAsync(); // Assert if (exception != null) @@ -65,7 +66,7 @@ public async Task WithAttachment() } } - [Fact(Timeout = 7000)] + [Fact] public async Task WorksInBackground() { // Arrange @@ -81,10 +82,16 @@ public async Task WorksInBackground() using var innerTransport = new FakeTransport(); await using var transport = CachingTransport.Create(innerTransport, options); + // Attach to the EnvelopeSent event. We'll wait for that below. + // ReSharper disable once AccessToDisposedClosure + using var waiter = new Waiter(handler => innerTransport.EnvelopeSent += handler); + // Act using var envelope = Envelope.FromEvent(new SentryEvent()); await transport.SendEnvelopeAsync(envelope); - await WaitForDirectoryToBecomeEmptyAsync(cacheDirectory.Path); + + // wait for the inner transport to signal that it sent the envelope + await waiter.WaitAsync(TimeSpan.FromSeconds(7)); // Assert var sentEnvelope = innerTransport.GetSentEnvelopes().Single(); @@ -184,8 +191,6 @@ public async Task ShouldLogOperationCanceledExceptionWhenNotIsCancellationReques [Fact] public async Task EnvelopeReachesInnerTransport() { - var timeout = TimeSpan.FromSeconds(7); - // Arrange using var cacheDirectory = new TempDirectory(); var options = new SentryOptions @@ -197,25 +202,20 @@ public async Task EnvelopeReachesInnerTransport() }; using var innerTransport = new FakeTransport(); - await using var transport = CachingTransport.Create(innerTransport, options); - - var tcs = new TaskCompletionSource(); - using var cts = new CancellationTokenSource(timeout); - innerTransport.EnvelopeSent += (_, _) => tcs.SetResult(true); - cts.Token.Register(() => tcs.TrySetCanceled()); + await using var transport = CachingTransport.Create(innerTransport, options, startWorker: false); // Act using var envelope = Envelope.FromEvent(new SentryEvent()); - await transport.SendEnvelopeAsync(envelope, cts.Token); - var completed = await tcs.Task; + await transport.SendEnvelopeAsync(envelope); + await transport.FlushAsync(); // Assert - Assert.True(completed, "The task timed out!"); var sentEnvelope = innerTransport.GetSentEnvelopes().Single(); - sentEnvelope.Should().BeEquivalentTo(envelope, o => o.Excluding(x => x.Items[0].Header)); + sentEnvelope.Should().BeEquivalentTo(envelope, + o => o.Excluding(x => x.Items[0].Header)); } - [Fact(Timeout = 5000)] + [Fact] public async Task MaintainsLimit() { // Arrange @@ -230,28 +230,20 @@ public async Task MaintainsLimit() }; var innerTransport = Substitute.For(); + await using var transport = CachingTransport.Create(innerTransport, options, startWorker: false); - var tcs = new TaskCompletionSource(); - - // Block until we're done - innerTransport - .When(t => t.SendEnvelopeAsync(Arg.Any(), Arg.Any())) - .Do(_ => tcs.Task.Wait()); - - await using var transport = CachingTransport.Create(innerTransport, options); - - // Act & assert + // Act for (var i = 0; i < options.MaxCacheItems + 2; i++) { using var envelope = Envelope.FromEvent(new SentryEvent()); await transport.SendEnvelopeAsync(envelope); - - transport.GetCacheLength().Should().BeLessOrEqualTo(options.MaxCacheItems); } - tcs.SetResult(null); + + // Assert + transport.GetCacheLength().Should().BeLessOrEqualTo(options.MaxCacheItems); } - [Fact(Timeout = 7000)] + [Fact] public async Task AwareOfExistingFiles() { // Arrange @@ -265,31 +257,32 @@ public async Task AwareOfExistingFiles() }; // Send some envelopes with a failing transport to make sure they all stay in cache + var initialInnerTransport = new FakeFailingTransport(); + await using var initialTransport = CachingTransport.Create(initialInnerTransport, options, startWorker: false); + + for (var i = 0; i < 3; i++) { - using var initialInnerTransport = new FakeTransport(); - await using var initialTransport = CachingTransport.Create(initialInnerTransport, options); + using var envelope = Envelope.FromEvent(new SentryEvent()); + await initialTransport.SendEnvelopeAsync(envelope); + } - // Shutdown the worker immediately so nothing gets processed - await initialTransport.StopWorkerAsync(); + // Move them all to processing and leave them there (due to FakeFailingTransport) + await initialTransport.FlushAsync(); - for (var i = 0; i < 3; i++) - { - using var envelope = Envelope.FromEvent(new SentryEvent()); - await initialTransport.SendEnvelopeAsync(envelope); - } - } + // Act + // Creating the transport should move files from processing during initialization. using var innerTransport = new FakeTransport(); - await using var transport = CachingTransport.Create(innerTransport, options); + await using var transport = CachingTransport.Create(innerTransport, options, startWorker: false); - // Act - await WaitForDirectoryToBecomeEmptyAsync(cacheDirectory.Path); + // Flushing the worker will ensure all files are processed. + await transport.FlushAsync(); // Assert innerTransport.GetSentEnvelopes().Should().HaveCount(3); } - [Fact(Timeout = 7000)] + [Fact] public async Task NonTransientExceptionShouldLog() { // Arrange @@ -308,10 +301,7 @@ public async Task NonTransientExceptionShouldLog() .SendEnvelopeAsync(Arg.Any(), Arg.Any()) .Returns(_ => Task.FromException(new Exception("The Message"))); - await using var transport = CachingTransport.Create(innerTransport, options); - - // Can't really reliably test this with a worker - await transport.StopWorkerAsync(); + await using var transport = CachingTransport.Create(innerTransport, options, startWorker: false); // Act using var envelope = Envelope.FromEvent(new SentryEvent()); @@ -328,7 +318,7 @@ public async Task NonTransientExceptionShouldLog() Assert.Equal("Failed to send cached envelope: {0}, discarding cached envelope. Envelope contents: {1}", message); } - [Fact(Timeout = 7000)] + [Fact] public async Task DoesNotRetryOnNonTransientExceptions() { // Arrange @@ -351,10 +341,7 @@ public async Task DoesNotRetryOnNonTransientExceptions() ? Task.FromException(new InvalidOperationException()) : Task.CompletedTask); - await using var transport = CachingTransport.Create(innerTransport, options); - - // Can't really reliably test this with a worker - await transport.StopWorkerAsync(); + await using var transport = CachingTransport.Create(innerTransport, options, startWorker: false); // Act for (var i = 0; i < 3; i++) @@ -375,28 +362,17 @@ public async Task DoesNotRetryOnNonTransientExceptions() _ = innerTransport.Received(0).SendEnvelopeAsync(Arg.Any(), Arg.Any()); } - [Fact(Timeout = 7000)] - public async Task DoesNotDeleteCacheIfHttpRequestException() - { - var exception = new HttpRequestException(null); - await TestNetworkException(exception); - } - - [Fact(Timeout = 7000)] - public async Task DoesNotDeleteCacheIfIOException() - { - var exception = new IOException(null); - await TestNetworkException(exception); - } - - [Fact(Timeout = 7000)] - public async Task DoesNotDeleteCacheIfSocketException() - { - var exception = new SocketException(); - await TestNetworkException(exception); - } + public static IEnumerable NetworkTestData => + new List + { + new object[] {new HttpRequestException(null)}, + new object[] {new IOException(null)}, + new object[] {new SocketException()} + }; - private async Task TestNetworkException(Exception exception) + [Theory] + [MemberData(nameof(NetworkTestData))] + public async Task TestNetworkException(Exception exception) { // Arrange using var cacheDirectory = new TempDirectory(); @@ -415,10 +391,7 @@ private async Task TestNetworkException(Exception exception) .SendEnvelopeAsync(Arg.Any(), Arg.Any()) .Returns(_ => Task.FromException(exception)); - await using var transport = CachingTransport.Create(innerTransport, options); - - // Can't really reliably test this with a worker - await transport.StopWorkerAsync(); + await using var transport = CachingTransport.Create(innerTransport, options, startWorker: false); using var envelope = Envelope.FromEvent(new SentryEvent()); await transport.SendEnvelopeAsync(envelope); @@ -443,40 +416,4 @@ private async Task TestNetworkException(Exception exception) Assert.Equal(exception, receivedException); Assert.True(Directory.EnumerateFiles(cacheDirectory.Path, "*", SearchOption.AllDirectories).Any()); } - - private async Task WaitForDirectoryToBecomeEmptyAsync(string directoryPath, TimeSpan? timeout = null) - { - bool DirectoryIsEmpty() => !Directory.EnumerateFiles(directoryPath, "*", SearchOption.AllDirectories).Any(); - - if (DirectoryIsEmpty()) - { - // No point in waiting if the directory is already empty - return; - } - - using var cts = new CancellationTokenSource(timeout ?? TimeSpan.FromSeconds(7)); - - using var watcher = new FileSystemWatcher(directoryPath); - watcher.IncludeSubdirectories = true; - watcher.EnableRaisingEvents = true; - - // Wait until timeout or directory is empty - while (!DirectoryIsEmpty()) - { - cts.Token.ThrowIfCancellationRequested(); - - var tcs = new TaskCompletionSource(); - cts.Token.Register(() => tcs.TrySetCanceled()); - watcher.Deleted += (_, _) => tcs.TrySetResult(true); - - // One final check before waiting - if (DirectoryIsEmpty()) - { - return; - } - - // Wait for a file to be deleted, but not longer than 100ms - await Task.WhenAny(tcs.Task, Task.Delay(100, cts.Token)); - } - } } diff --git a/test/Sentry.Tests/SentrySdkTests.cs b/test/Sentry.Tests/SentrySdkTests.cs index 5ed2d0cfb5..fb6c6d5018 100644 --- a/test/Sentry.Tests/SentrySdkTests.cs +++ b/test/Sentry.Tests/SentrySdkTests.cs @@ -247,10 +247,7 @@ public async Task Init_WithCache_BlocksUntilExistingCacheIsFlushed() DiagnosticLogger = _logger, Dsn = ValidDsnWithoutSecret, CacheDirectoryPath = cacheDirectory.Path - }); - - // Shutdown the worker to make sure nothing gets processed - await initialTransport.StopWorkerAsync(); + }, startWorker: false); for (var i = 0; i < 3; i++) {