From 20d0106785eb6749abd475e73298cb4904341245 Mon Sep 17 00:00:00 2001 From: Jericho Date: Tue, 10 Dec 2024 13:32:36 -0500 Subject: [PATCH 1/5] Fix exception when disposing BetterStackLoggerProvider --- .../BetterStackLoggerProvider.cs | 61 +++++++++++++------ 1 file changed, 42 insertions(+), 19 deletions(-) diff --git a/src/Formitable.BetterStack.Logger.Microsoft/BetterStackLoggerProvider.cs b/src/Formitable.BetterStack.Logger.Microsoft/BetterStackLoggerProvider.cs index e6e719c..28ffdd6 100644 --- a/src/Formitable.BetterStack.Logger.Microsoft/BetterStackLoggerProvider.cs +++ b/src/Formitable.BetterStack.Logger.Microsoft/BetterStackLoggerProvider.cs @@ -50,46 +50,65 @@ internal void EnqueueLogEvent(BetterStackLogEnvelope logEnvelope) } } + // This task runs continously until the cancellation token is canceled private async Task FlushQueue() { while (!_cancellationTokenSource.IsCancellationRequested) { - var limit = _currentConfig.BatchSize; - var batch = new List(); - - while (limit > 0 && _logQueue.TryTake(out var message)) + int messagesFlushed = await FlushBatch().ConfigureAwait(false); + if (messagesFlushed == 0) { - batch.Add(message); - limit--; + await Task.Delay(_currentConfig.FlushFrequency, _cancellationTokenSource.Token); } + } + } + + // This method uploads a batch of messages to betterstack.com + private async Task FlushBatch() + { + var limit = _currentConfig.BatchSize; + var batch = new List(); + + while (limit > 0 && _logQueue.TryTake(out var message)) + { + batch.Add(message); + limit--; + } - if (batch.Any()) + if (batch.Any()) + { + try { - try - { - await _client.UploadAsync(batch, _cancellationTokenSource.Token); - } - catch (Exception ex) - { - Console.WriteLine($"Failed to upload logs to BetterStack: {ex.Message}"); - } + await _client.UploadAsync(batch, _cancellationTokenSource.Token); } - else + catch (Exception ex) { - await Task.Delay(_currentConfig.FlushFrequency, _cancellationTokenSource.Token); + Console.WriteLine($"Failed to upload logs to BetterStack: {ex.Message}"); } } + + return batch.Count; } public void Dispose() { _loggers.Clear(); _onChangeToken?.Dispose(); - _cancellationTokenSource.Dispose(); + // Stop "_flushTask" and pause briefly to ensure it completes + _cancellationTokenSource.Cancel(); + Task.Delay(TimeSpan.FromMilliseconds(250)); + + // There's a possibility that some messages could still be in the queue. + // Therefore we need to flush these remaining messages. try { - _flushTask.Wait(_currentConfig.FlushFrequency); + // Flush any remaining messages until the queue is empty + int batchCount = 0; + do + { + batchCount = FlushBatch().GetAwaiter().GetResult(); + } while (batchCount > 0); } catch (TaskCanceledException) { @@ -97,6 +116,10 @@ public void Dispose() catch (AggregateException ex) when (ex.InnerExceptions.Count == 1 && ex.InnerExceptions[0] is TaskCanceledException) { } + finally + { + _cancellationTokenSource.Dispose(); + } } public void SetScopeProvider(IExternalScopeProvider scopeProvider) => ScopeProvider = scopeProvider; From b8b860c98a9cc371e32e8f3aee96ee3f16768db3 Mon Sep 17 00:00:00 2001 From: Jericho Date: Fri, 13 Dec 2024 10:17:29 -0500 Subject: [PATCH 2/5] Make sure we invoke ".ConfigureAwait(false)" whenever making an async call --- .../BetterStackLoggerProvider.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Formitable.BetterStack.Logger.Microsoft/BetterStackLoggerProvider.cs b/src/Formitable.BetterStack.Logger.Microsoft/BetterStackLoggerProvider.cs index 28ffdd6..4f4325c 100644 --- a/src/Formitable.BetterStack.Logger.Microsoft/BetterStackLoggerProvider.cs +++ b/src/Formitable.BetterStack.Logger.Microsoft/BetterStackLoggerProvider.cs @@ -58,7 +58,7 @@ private async Task FlushQueue() int messagesFlushed = await FlushBatch().ConfigureAwait(false); if (messagesFlushed == 0) { - await Task.Delay(_currentConfig.FlushFrequency, _cancellationTokenSource.Token); + await Task.Delay(_currentConfig.FlushFrequency, _cancellationTokenSource.Token).ConfigureAwait(false); } } } @@ -79,7 +79,7 @@ private async Task FlushBatch() { try { - await _client.UploadAsync(batch, _cancellationTokenSource.Token); + await _client.UploadAsync(batch, _cancellationTokenSource.Token).ConfigureAwait(false); } catch (Exception ex) { From a57f5062a553051bac804bbe47545e7e50deb05f Mon Sep 17 00:00:00 2001 From: Jericho Date: Wed, 15 Jan 2025 15:57:20 -0500 Subject: [PATCH 3/5] Ensure a given batch of messages is uploaded to BetterStack.com even when the cancellation token is canceled --- .../BetterStackLoggerProvider.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Formitable.BetterStack.Logger.Microsoft/BetterStackLoggerProvider.cs b/src/Formitable.BetterStack.Logger.Microsoft/BetterStackLoggerProvider.cs index 4f4325c..27087df 100644 --- a/src/Formitable.BetterStack.Logger.Microsoft/BetterStackLoggerProvider.cs +++ b/src/Formitable.BetterStack.Logger.Microsoft/BetterStackLoggerProvider.cs @@ -79,7 +79,7 @@ private async Task FlushBatch() { try { - await _client.UploadAsync(batch, _cancellationTokenSource.Token).ConfigureAwait(false); + await _client.UploadAsync(batch, CancellationToken.None).ConfigureAwait(false); } catch (Exception ex) { From 3532c0d9e92a71dc5386cba0940d9d2019c86466 Mon Sep 17 00:00:00 2001 From: Jericho Date: Thu, 16 Jan 2025 09:50:25 -0500 Subject: [PATCH 4/5] Comment to explain why using CancellationToken.None rather than _cancellationTokenSource.Token is deliberate --- .../BetterStackLoggerProvider.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Formitable.BetterStack.Logger.Microsoft/BetterStackLoggerProvider.cs b/src/Formitable.BetterStack.Logger.Microsoft/BetterStackLoggerProvider.cs index 27087df..d55b361 100644 --- a/src/Formitable.BetterStack.Logger.Microsoft/BetterStackLoggerProvider.cs +++ b/src/Formitable.BetterStack.Logger.Microsoft/BetterStackLoggerProvider.cs @@ -79,6 +79,10 @@ private async Task FlushBatch() { try { + // Cancellation.None on the following line might seem counterintuitive, but it's + // actually quite important. We want to ensure the upload of a given batch of + // messages completes sucessfully even when _cancellationTokenSource.Token is + // cancelled (which indicates that BetterStackLoggerProvider is being disposed). await _client.UploadAsync(batch, CancellationToken.None).ConfigureAwait(false); } catch (Exception ex) From 6c4726b03da7391727f9e39bd33210452e28c609 Mon Sep 17 00:00:00 2001 From: Jericho Date: Sun, 19 Jan 2025 10:04:52 -0500 Subject: [PATCH 5/5] Use Task.Wait rather than Task.Delay to wait for the "flush task" to complete Also improve XML comments --- .../BetterStackLoggerProvider.cs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/Formitable.BetterStack.Logger.Microsoft/BetterStackLoggerProvider.cs b/src/Formitable.BetterStack.Logger.Microsoft/BetterStackLoggerProvider.cs index d55b361..315ccd2 100644 --- a/src/Formitable.BetterStack.Logger.Microsoft/BetterStackLoggerProvider.cs +++ b/src/Formitable.BetterStack.Logger.Microsoft/BetterStackLoggerProvider.cs @@ -99,15 +99,16 @@ public void Dispose() _loggers.Clear(); _onChangeToken?.Dispose(); - // Stop "_flushTask" and pause briefly to ensure it completes + // Stop "_flushTask" and wait for it completes _cancellationTokenSource.Cancel(); - Task.Delay(TimeSpan.FromMilliseconds(250)); + _flushTask.Wait(TimeSpan.FromMilliseconds(500)); - // There's a possibility that some messages could still be in the queue. - // Therefore we need to flush these remaining messages. + // There's a possibility the Task was stoped before the queue + // could be fully processed. Therefore we need to flush any + // remaining messages. try { - // Flush any remaining messages until the queue is empty + // Flush remaining messages until the queue is empty int batchCount = 0; do {