Skip to content
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

null out _unsubscribe after unsubscription #5465

Merged
merged 7 commits into from
Jul 30, 2020

Conversation

felixfbecker
Copy link
Contributor

@felixfbecker felixfbecker commented Jun 1, 2020

Description:
null out the unsubscribe callback after unsubscribing.

The any is kind of ugly but that is also how it's assigned in the constructor.

Related issue (if exists): Fixes #5464

@felixfbecker
Copy link
Contributor Author

Okay this seems to break tests, don't know enough to proceed. I'd appreciate if a maintainer could look at this bug 🙇

@cartant
Copy link
Collaborator

cartant commented Jun 1, 2020

Could you add a test to show/prove that multiple unsubscribe calls are okay and that overwriting the _unsubscribe method in Subscriber with null - which is what this change will end up doing - behaves as expected?

@cartant
Copy link
Collaborator

cartant commented Jun 1, 2020

Okay this seems to break tests, don't know enough to proceed. I'd appreciate if a maintainer could look at this bug.

It's likely related to my previous comment. I'll have a look at it. Thanks for the PR and the bug report.

@cartant cartant self-assigned this Jun 1, 2020
@cartant cartant requested a review from benlesh June 1, 2020 23:04
Copy link
Contributor Author

@felixfbecker felixfbecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@felixfbecker
Copy link
Contributor Author

@benlesh would you mind giving this a review when you have the chance? 🙌

// 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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this cause a de-opt in V8 though, i.e. put the entire Subscription instance in dictionary mode?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick fix could also be to make the constructor just call this.add(teardown) instead of assigning it to _unsubscribe

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I considered that, but, IIRC, the implementation of add creates another Subscription - passing the teardown to the ctor.

Copy link
Collaborator

@cartant cartant Jun 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cartant
Copy link
Collaborator

cartant commented Jul 26, 2020

@benlesh I've requested your review on this. It's a small change to Subscription to ensure that the reference to the teardown passed to the ctor is released/nulled upon unsubscription.

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 _unsubscribe is nulled.

Also, the tests that are added in this PR don't fail without the PR's change to the Subscription class. They were added when I was trying to figure out why nulling _unsubscribe was effecting failures in other tests. I couldn't see specific tests for unsubscribe idempotency, so I figured they could remain in the PR for inclusion in the test suite.

Copy link
Member

@benlesh benlesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh. The inheritance trickery in this library is the worst. That's all going away someday.

@benlesh benlesh merged commit 3e39749 into ReactiveX:master Jul 30, 2020
benlesh pushed a commit that referenced this pull request Jul 30, 2020
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subscription callback is not nulled out when Subscription is unsubscribed
3 participants