[Proposal] Make SharedReference's clearSignal param mandatory #1592
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOTE: Most updates here are prettier being prettier and re-indenting things. No logic beside one handling the
clearSignal
parameter should have been updated.We've seen minor memory leaks in the project due to forgetting to clean-up what's basically event listeners (mainly JS-only ones, not DOM ones).
To put a supplementary level of security against memory leaks, I propose here to make the
SharedReference
and anyIPlaybackObserver
'sclearSignal
parameter as mandatory.Note that this is not always needed to clean up a listener, there's for example the
stopListening
argument provided in the callback, "finishing" the reference etc., so a reference without a CancellationSignal could still not be leaking, but ensuring there's a CancellationSignal also attached looks like a minor sacrifice to limit memory leaks to me.