null out _unsubscribe after unsubscription #5465
Conversation
|
Okay this seems to break tests, don't know enough to proceed. I'd appreciate if a maintainer could look at this bug 🙇 |
|
Could you add a test to show/prove that multiple |
It's likely related to my previous comment. I'll have a look at it. Thanks for the PR and the bug report. |
|
@benlesh would you mind giving this a review when you have the chance? 🙌 |
src/internal/Subscription.ts
Outdated
| // are recycled. Deleting the member will release the reference to any | ||
| // teardown functions passed in the constructor and will leave any | ||
| // methods intact. | ||
| delete (this as any)._unsubscribe; |
There was a problem hiding this comment.
Won't this cause a de-opt in V8 though, i.e. put the entire Subscription instance in dictionary mode?
There was a problem hiding this comment.
Yeah. We should really implement an _unsubscribe method that's always present and store the teardown that's passed to the ctor in a property that's actually writable.
There was a problem hiding this comment.
A quick fix could also be to make the constructor just call this.add(teardown) instead of assigning it to _unsubscribe
There was a problem hiding this comment.
Yeah. I considered that, but, IIRC, the implementation of add creates another Subscription - passing the teardown to the ctor.
There was a problem hiding this comment.
I wasn't too happy about creating a closure in the ctor, either. I think we should just bite the bullet and add the method. As it is ATM, assigning to _unsubscribe on the instance when it's a method elsewhere is confusing AF.
There was a problem hiding this comment.
Given that the Subscription implementation does so much other clean up, I suppose it make some sense to release the _unsubscribe teardown, too, but - I'm curious - is there some particular use case in which you want to continue to hold the subscription after you've called unsubscribe? I mean, if it's inside - i.e. was added to - another longer-lived subscription, the unsubscription of the shorter-lived subscription should remove it from the longer-lived one, so I'm wondering about the use case and whether or not there's a bug someplace else.
There was a problem hiding this comment.
I have a fairly large codebase that uses web workers and MessagePorts, which are notorious for needing manual memory management. This code is also running inside a browser extension in a background page (i.e. a long-living context) and will create new ports very often for new tabs, proxying objects within that tab, etc. Given these attributes, I want to do everything in my power to make sure there is no memory leaking. If _unsubscribe is not nulled out, I can't guarantee that, because it is very easy to accidentally break out of the nice parent-cleaning tree and there's no way to ensure this at scale. For example, somewhere in the code someone may have written
subscription.add(() => otherSubscription.unsubscribe())instead of
subscription.add(otherSubscription)and the nice parent-cleaning chain would be broken, and the parent would hold on to the callback. This is a simple example, but this could also happen with more layers in between, e.g. a class that has an internal Subscription bag, but exposes an unsubscribe or close method itself, that is then added to a Subscription. It should be possible to rely on at least the low-level to clean things up properly, so that we may hold a reference to an empty Subscription longer than needed, but at least the underlying MessagePort and heavier resources like the web worker etc are cleaned up.
|
@benlesh I've requested your review on this. It's a small change to Felix's explanation for this is here. Given that the subscription implementation goes to so much effort to remove closed subscriptions, this kinda makes sense, to me. FWIW, various attempts at implementing this change ran into the inheritance weirdness/abuse that you mentioned, recently. There is a comment regarding this where Also, the tests that are added in this PR don't fail without the PR's change to the |
…mpotent (#5465) * null out _unsubscribe after unsubscription Fixes #5464 * test: add idempotent unsubscribe tests * fix: null the subscription ctor teardown * refactor: use delete to remove the member * refactor: delete _unsubscribe only if it exists * refactor: replace delete with hasOwnProperty etc * refactor: use _ctorUnsubscribe flag Co-authored-by: Nicholas Jamieson <nicholas@cartant.com>
Description:
nullout theunsubscribecallback after unsubscribing.The
anyis kind of ugly but that is also how it's assigned in the constructor.Related issue (if exists): Fixes #5464