-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix(Subscriber): handle memory leaks #5934
fix(Subscriber): handle memory leaks #5934
Conversation
a6d0164
to
8058b46
Compare
@benlesh Hi. |
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.
One thing is for sure, we don't want to change the semantics of the test I commented on. Overall I see what's being done here and it makes sense.
I think we really want to test destination
to null
though, and then maybe even have a test that confirms that is the case via introspection? The test wouldn't be super valuable, other than to serve as a reminder that we want to be mindful of that.
@@ -198,7 +198,7 @@ describe('bufferWhen operator', () => { | |||
const result = e1.pipe( | |||
bufferWhen(() => { | |||
if (i === 1) { | |||
throw 'error'; | |||
return throwError(() => 'error'); |
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.
This completely changes the test. Revert.
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.
@benlesh Yeah I assumed this wasn't the right way to go about it, but did it on the off-chance that the test itself was previously incorrect (as I figured that bufferWhen
's closingSelector
was supposed to return an observable).
@@ -106,6 +106,7 @@ export class Subscriber<T> extends Subscription implements Observer<T> { | |||
if (!this.closed) { | |||
this.isStopped = true; | |||
super.unsubscribe(); | |||
this.destination = NOOP_OBSERVER; |
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.
at the point we've unsubscribed, it seems like we could just set this.destination = null!
here, although I haven't tried that and tested.
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.
I believe simply nulling it out caused several tests to fail.
/** | ||
* The observer used as a stub for subscriptions that have already been closed. | ||
*/ | ||
export const NOOP_OBSERVER: Readonly<Observer<any>> & { closed: true } = { |
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.
We can probably remove this if we're just nulling out destination
.
@@ -66,6 +66,7 @@ export class Subscription implements SubscriptionLike { | |||
} | |||
|
|||
const { initialTeardown } = this; | |||
this.initialTeardown = undefined; |
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.
I'd use null!
here, I think (not sold on it), just because null
tells me that we set it, rather than it just never being set.
Okay, I'm looking more into this. It would be great to have a reproduction that didn't involve Angular. But I'm totally certain that the code here isn't the right solution. Using the "noop observer" that this adds causes errors we'd like to propagate somehow to simply be swallowed. Again, I think I see what you're getting at, but it's hard without a minimal reproduction, and it's also plausible that this is something that only has to do with the Noteworthy: I'm moving back to trying to find a minimal reproduction for this first. That should be our starting point. cc @cartant |
I'm going to close this for now, because I don't want to send the wrong signal to the author, @MaximSagan, to continue working on it. What we really need right now is a minimal, RxJS only, reproduction that definitively shows the leak. I'll try to figure that out too. |
Also to be clear: I'm not saying that there isn't a leak. I'm saying that we need to more definitively identify the cause of the leak we think exists rather than blindly address the symptom. |
@benlesh Thanks for looking into this. Not offended that you're not going with my solution, haha. Really just wanted to get this issue on the radar, and I figured a PR might help. |
@MaximSagan one thing to look for when trying to figure out what is or isn't cleaning up is tests with an |
Description:
Fixes memory leaks occurring when instances of
Subscriber
are kept around in memory even after unsubscription (which includescomplete()
anderror()
).Related issue (if exists):
#5931