getsentry
feat(core): allow unregistering callback through `on`
This commit updates the return signature of the client's on function to be a void function,
which, when executed, unregisters a callback. This adjustment is necessary for managing instances
where objects are created and destroyed, ensuring that callbacks are properly unregistered to
prevent self-referencing in callback closures and facilitate proper garbage collection.
Typically, changing a type from void to () => void (or VoidFunction) shouldn't be considered
a breaking change because void signifies the absence of a return value, implying that the return
value of the on function should never be used by consumers.
Opting for the on approach, which returns a cleanup function, "seems" simpler because having another
function called off requires saving the callback reference for later removal. With our pattern,
we encapsulate both the registration and removal of event listeners within a single function call.
While I can see where this is coming from, so far we decided against this to reduce complexity there. We generally do not do a lot of cleanup in a lot of places because (esp. in browser) because we make assumptions about the environment where something runs. I am not an Angular expect (cc @Lms24 ), but shouldn't the goal be to rather avoid the error handler to be continuously torn down and re-created? To me, it seems like this is the optimization that should be done, not to cleanup the one lone handler that is registered there.
Now, I don't have a problem per-se to add this change, the main reason we do not have cleanup everywhere is because it adds bundle size (not necessarily this change itself, but ensuring everywhere that stuff is properly cleaned up can sum up to a not-so-small amount of code, sadly). Not saying we shouldn't do this, just thinking out loud... 🤔
Hey @mydea
Well, it's generally considered good practice to have a cleanup functional API, akin to addEventListener and removeEventListener. Initially, I pondered adding an off function instead of modifying on. However, as you pointed out, this approach seems to introduce additional complexity compared to enhancing the existing on function to return a cleanup function directly tied to the added callback. This approach avoids a redundant "abstraction" where a callback needs to be stored in a variable for later removal, as is the case with removeEventListener, which requires a reference to the callback.
What you mentioned about the Angular error handler also applies to other frameworks that incorporate server-side rendering and could potentially extend to microfrontend applications. Take, for example, a React application that is rendered on the server side and destroyed with each HTTP request. Similarly, in the context of microfrontend applications, consider an app associated with a specific URL segment, such as /blog. When navigating away from that URL, the corresponding app instance needs to be properly destroyed and all of the resources should be cleaned up.
Hey @arturovt thanks for opening this PR!
Regarding the unsubscription mechanism we need to discuss this more thoroughly to decide what we're doing here. I'd tend to agree with @mydea that ideally we shouldn't add the extra bytes for correctly handling unsbscribing if at all possible. However, we should of course find a solution that doesn't cause hundreds of subscriptions from the errorHandler (or in other situations as you pointed out).
Consider, for instance, a scenario in Angular where the error handler is created and destroyed with each app rendering cycle.
Is this really default behaviour in Angular? If so there's probably a good reason for it but on first glance it's a bit unintuitive/unnecessary to me 🤔 Would appreciate any context/knowledge around this, thx :)
Also a general question: Is there a concrete reason for opening the PR? For example, did the SDK cause some kind of out of memory error that can be attributed to the client.on hooks? I just want to assess impact and priority for finding a good path forward.
@Lms24 This solution, lacking the functionality to remove an event listener when certain components are destroyed, such as any class with 'disposable' functionality, may inadvertently lead to memory leaks. When a callback captures local variables, there's no issue of leaks. However, if a callback captures this, it creates a closure that strongly retains a reference to this, preventing its garbage collection.
Describing the technical necessity behind why this is crucial can be somewhat challenging, especially when explaining the importance of removeEventListener.
Considering an Angular example: when the root injector is destroyed, it also destroyes all injectees it maintains as records. You may observe a console.error coming from ngOnDestroy (hook which is invoked when the root injector is destroyed, I added it intentionally). And at the end, you may also observe the error handler has not been GCd because its reference is retained in client hooks:
Hey, we just noticed that this kind of fell of the table in between reviews - sorry about that! Would you mind rebasing on develop and resolving any conflicts, then I think this should be ready for a final review & merge!
Hey, we just noticed that this kind of fell of the table in between reviews - sorry about that! Would you mind rebasing on develop and resolving any conflicts, then I think this should be ready for a final review & merge!
Done.