-
-
Notifications
You must be signed in to change notification settings - Fork 226
fix(logs): remove Dispose from the public API surface area #4403
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
|
@sentry review |
| public void Dispose() | ||
| { | ||
| if (disposing) | ||
| { | ||
| _batchProcessor.Dispose(); | ||
| } | ||
|
|
||
| base.Dispose(disposing); | ||
| _batchProcessor.Dispose(); | ||
| } |
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 simplified Dispose() implementation is cleaner, but consider adding a disposed flag to prevent double disposal. The current implementation will call _batchProcessor.Dispose() multiple times if Dispose() is called repeatedly, which could potentially cause issues depending on the StructuredLogBatchProcessor implementation.
| public void Dispose() | |
| { | |
| if (disposing) | |
| { | |
| _batchProcessor.Dispose(); | |
| } | |
| base.Dispose(disposing); | |
| _batchProcessor.Dispose(); | |
| } | |
| private bool _disposed;public void Dispose(){if (_disposed){return;}_batchProcessor.Dispose();_disposed = true;} |
Did we get this right? 👍 / 👎 to inform future reviews.
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 only fields of DefaultSentryStructuredLogger that require cleanup is the StructuredLogBatchProcessor, which owns two IDisposable fields of type StructuredLogBatchBuffer.
According to the Guidelines:
To help ensure that resources are always cleaned up appropriately, a Dispose method should be idempotent, such that it's callable multiple times without throwing an exception. Furthermore, subsequent invocations of Dispose should do nothing.
See https://learn.microsoft.com/dotnet/standard/garbage-collection/implementing-dispose
We do own the implementation of the _batchProcessor field, where we can also ensure the Guideline is adhered to. So adding a private bool _disposed field to this type is possible, but not necessary.
I also double-checked the IDisposable implementation of the StructuredLogBatchBuffer type, and the IDisposable fields that it contains ... and all types do satisfy the Guideline.
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.
Note for Seer:
Diff
The suggestion is created off of the base / old code.
I expected the suggestion to be created off of the change / new code.
Formatting (C#)
The formatting of the suggested change is not idiomatic C# code.
It is suggested as a one-liner, where it should be suggested as such:
-private bool _disposed;public void Dispose(){if (_disposed){return;}_batchProcessor.Dispose();_disposed = true;}
+private bool _disposed;
+
+public void Dispose()
+{
+ if (_disposed)
+ {
+ return;
+ }
+
+ _batchProcessor.Dispose();
+ _disposed = true;
+}Note
Technically, the suggestion is correct!
I opt to dismiss is though because it is not necessary in this case.
closed
superseded by #4424