-
Notifications
You must be signed in to change notification settings - Fork 11
Log dispose exceptions to unity logger #16
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
base: main
Are you sure you want to change the base?
Conversation
| { | ||
| disposable?.Dispose(); | ||
| } | ||
| catch (AggregateException e) |
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.
Can you please add light optimization by returning from this method when disposables has 0 elements?
Also, I'm thinking about making _compositeDisposables in the ControllerBase to be null by default and pooled only during first attempt to add disposable. Wdyt?
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.
There are no simple ways to check the number of elements in an IEnumerable for the AddRange method. However, for internal _disposables object, seem like a suitable option.
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.
Also did lazy ControllerCompositeDisposable creation in ControllerBase. Pleasy, take a look.
eldarmguseynov
left a comment
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.
Thank you for the fix. What do you think about optimizations around disposables?
I will be honest with you, I didn't think about optimization here too much. Right now, it looks pretty optimized, as I see. Espesially after small tweaks you proposed in method DisposeMany and ControllerBase class. |
|
@eldarmguseynov hello! Is there any chance that you will merge this anytime soon? |
|
Related to https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1065 Dispose should not throw any exception. As for the use of Debug.LogError, many projects use their own logger, and we avoided using Debug.LogError in the controllers core. The idea of a lazy composite disposable object is good, and we can use pooling instead of new. @eldarmguseynov what do you think? |
Exception re-throw fix
In a previous PR I added an optimization that unwrapped a single dispose failure (
case 1: throw exceptionList[0];). That was a bad approach because re-throwing a caught exception instance replaces the original stack trace with the dispose call stack. Keeping failures as inner exceptions preserves their original stack traces. Now it is removedWhat changed in this PR
IDisposable.Dispose()calls are now logged viaDebug.LogException(...).ControllerDisposeAggregateExceptionand always throw it when any dispose errors occur. When an inner composite throws this marker, the outer composite collects itsInnerExceptions(leaf exceptions) and skips logging the marker again, preventing duplicate console entries in nested hierarchies.Tests
ControllerDisposeAggregateException(including nested composite scenarios).LogAssert.Expect(...)where dispose exceptions are now logged.InternalsVisibleTo("Playtika.Controllers.Tests")for asserting the internal marker type.