-
-
Notifications
You must be signed in to change notification settings - Fork 226
fix(logs): flush on terminating #4402
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
Conversation
…sentry-dotnet into feat/logs-initial-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Logger Flush Timeout Issues
The Logger.Flush call in Hub.FlushAsync now unconditionally uses TimeSpan.Zero. This causes two issues:
- In debug builds,
Debug.Assert(_addLock.IsSet)fails inStructuredLogBatchBuffer.FlushScope.Flush(TimeSpan timeout)if the timeout (e.g.,TimeSpan.Zero) elapses before allAddoperations complete, particularly during ungraceful shutdowns. - For normal
FlushAsynccalls, usingTimeSpan.Zeroprevents waiting for pending log additions, potentially leading to data loss or race conditions. This is inconsistent withHub.Disposewhich usesTimeout.InfiniteTimeSpanfor safe flushing.
src/Sentry/Internal/StructuredLogBatchBuffer.cs#L196-L197
sentry-dotnet/src/Sentry/Internal/StructuredLogBatchBuffer.cs
Lines 196 to 197 in 20eba58
| private void Clear(int length) |
src/Sentry/Internal/Hub.cs#L788-L789
sentry-dotnet/src/Sentry/Internal/Hub.cs
Lines 788 to 789 in 20eba58
| { | |
| Logger.Flush(TimeSpan.Zero); |
| { | ||
| try | ||
| { | ||
| Logger.Flush(TimeSpan.Zero); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Logger Flush Ignores Timeout Parameter
The FlushAsync method inconsistently applies its timeout parameter. Logger.Flush is hardcoded to TimeSpan.Zero, causing it to return immediately, while CurrentClient.FlushAsync correctly uses the provided timeout. This prevents the logger from gracefully flushing, leading to an incomplete or ungraceful overall flush operation.
Locations (1)
|
@sentry review |
| { | ||
| Logger.Flush(TimeSpan.Zero); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The choice to use TimeSpan.Zero for logger flush in FlushAsync is questionable. This means the logger flush will never wait and could return immediately without actually flushing logs, potentially causing data loss. Consider using a small timeout (e.g., TimeSpan.FromMilliseconds(100)) or a fraction of the main timeout to ensure logs are actually flushed while not blocking indefinitely.
Did we get this right? 👍 / 👎 to inform future reviews.
| /// <param name="timeout">A <see cref="TimeSpan"/> that represents the number of milliseconds to wait, or a <see cref="TimeSpan"/> that represents <see langword="-1"/> milliseconds to wait indefinitely.</param> | ||
| /// <exception cref="System.ArgumentOutOfRangeException"><paramref name="timeout"/> is a negative number other than <see langword="-1"/> milliseconds, which represents an infinite time-out -or- <paramref name="timeout"/> is greater than <see cref="int.MaxValue"/>.</exception> | ||
| internal void Wait(TimeSpan timeout) | ||
| { | ||
| var lockObj = _lockObj; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Wait(TimeSpan timeout) method calls CountdownEvent.Wait(timeout, CancellationToken.None) but doesn't handle or propagate the return value. According to .NET documentation, CountdownEvent.Wait returns false when the timeout expires. Callers have no way to know if the wait succeeded or timed out, which could lead to incorrect behavior in flush operations.
Did we get this right? 👍 / 👎 to inform future reviews.
|
|
||
| internal SentryLog[] Flush(TimeSpan timeout) | ||
| { | ||
| var lockObj = _lockObj; | ||
| if (lockObj is not null) | ||
| { | ||
| _scope.Wait(); | ||
| _scope.Wait(timeout); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FlushScope.Flush(TimeSpan timeout) method doesn't handle the case where _scope.Wait(timeout) times out. If the wait times out, the method continues to call ToArrayAndClear(), which could lead to data corruption if add operations are still in progress. The method should check if the wait succeeded before proceeding.
Did we get this right? 👍 / 👎 to inform future reviews.
| { | ||
| SwapActiveBuffer(buffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timeout parameter is passed to CaptureLogs but there's an inconsistency where OnTimeoutExceeded still uses Timeout.InfiniteTimeSpan. This could lead to deadlocks if the buffer is in a bad state. Consider using the same timeout handling strategy consistently.
Did we get this right? 👍 / 👎 to inform future reviews.
| SwapActiveBuffer(buffer); | ||
| CaptureLogs(buffer); | ||
| CaptureLogs(buffer, Timeout.InfiniteTimeSpan); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the TryEnqueue method, when status is AddedLast, the flush operation uses Timeout.InfiniteTimeSpan which could cause the application to hang if there are threading issues. Consider using a reasonable timeout here as well to prevent potential deadlocks.
Did we get this right? 👍 / 👎 to inform future reviews.
closed
superseded by #4425