I tested this locally, and the changes in this PR don't fix the memory leak reported on #5931, so I'm closing it for now. I will re-open it if I find a solution.

Actually, sorry, it does fix the memory leak

@josepot @benlesh
I believe this PR should be merged as there is no reason to keep those subscription objects around any longer than necessary, but I think it doesn't get to the root of the problem here.

That is, we have to ask why an instance of Subscriber (which is what Observable.prototype.subscribe(observer) returns) is maintaining any reference at all to its Observer after unsubscription (including complete() & error()).

With that in mind, I have made this related PR, so that this kind of thing will (hopefully) never be a problem again for any operator (not just shareReplay): #5934

I'm going to close this PR because the changes don't really do anything (as I outlined above). If there's proof that this changes the memory usage, I'll be happy to take another look.

I'm sorry if I'm being annoying, but @benlesh or @cartant could you please have a look at this codesandbox? 🙏. It's simply doing a of(1).pipe(shareReplay()).subscribe() while using the current version of shareReplay on v6 with 2 console.logs that illustrate why the if (isComplete) statement that I suggested in this PR is necessary in order to avoid the memory leak.

Also, thanks a lot for the amazing work that you are doing on v7. I've been using it on a project of mine and so far things are looking great! There is just one little thing that's annoying me: which is that I'm seeing code in a chunk that I really think that it should have been "tree shaken" and it wasn't. I'm still investigating, I will open an issue or a discussion in the coming days to explain this better and I will make sure that when I open the issue I can give you as much info as I can. Thanks again!

@josepot I'll have a look at this in a bit. I'll push a test to this branch to clear things up.

Derp. @josepot is correct. I overlooked that synchronously, subscription is set after it's set to undefined when the subscribe call returns. Reopening this.

Add a comment, as suggested, above the new conditional.

✔️

Add a test that proves this resolves the issue.

Is that possible without testing the internals? 🤔 I mean, the only way that I can think off, involves exposing an implementation detail... which IMO wouldn't be worth it. I'm very open to suggestions.

EDIT: I found this tool, but I don't see it in the list of dev-dependencies... Maybe you are using a similar tool/approach in other tests? If that's the case, could you please point me in the right direction? 🙏 I'd love to learn how to properly test for memory leaks. Thanks!

Is that possible without testing the internals? 🤔

Yes. I'll add one soon.

@josepot @benlesh I've added a leak test for shareReplay.

To effect a failure, you will have to comment out the nulling of innerSub and subscription.

Note that FinalizationRegistry is only in Node 14 and later. I'll add a similar test to master soon.

IDK what these build failures are. I hate CI so much.

@cartant hats off to you for that test!! 👏 👏 👏 TIL about FinalizationRegistry, amazing! Thanks a lot!

PS @voliva check this out we should start using this on @react-rxjs/core ASAP.

@josepot IDK whether or not you can see the details of the CI failure, but this is what's breaking:

Error: node_modules/@types/node/index.d.ts(74,11): error TS2300: Duplicate identifier 'IteratorResult'.
Error: node_modules/typescript/lib/lib.es2015.iterable.d.ts(41,6): error TS2300: Duplicate identifier 'IteratorResult'.
npm ERR! code 2
npm ERR! path /home/runner/work/rxjs/rxjs
npm ERR! command failed
npm ERR! command sh -c tsc -p ./tsconfig/compat/tsconfig.cjs.json

🤷‍♂️ Maybe there is was a breaking change in the node types that match whatever semver is in this branch's package.json. IDK.

@josepot IDK whether or not you can see the details of the CI failure, but this is what's breaking:

Error: node_modules/@types/node/index.d.ts(74,11): error TS2300: Duplicate identifier 'IteratorResult'.
Error: node_modules/typescript/lib/lib.es2015.iterable.d.ts(41,6): error TS2300: Duplicate identifier 'IteratorResult'.
npm ERR! code 2
npm ERR! path /home/runner/work/rxjs/rxjs
npm ERR! command failed
npm ERR! command sh -c tsc -p ./tsconfig/compat/tsconfig.cjs.json

Maybe there is was a breaking change in the node types that match whatever semver is in this branch's package.json. IDK.

Thanks a lot @cartant ! Yep, I can see that. There were 2 different checks failing, one has been fixed by my latest commit (a linting issue, aparently). And this one, I'm investigating... The first thing that I tried was rebasing the branch from 6.x, just in case, but that didn't help... And yeah, based on a quick google search that error normally indicates an issue with the node types. I will investigate a bit more later. Thanks again.

man_shrugging Maybe there is was a breaking change in the node types that match whatever semver is in this branch's package.json. IDK.

Yep, I think that's the issue... Because -according to the logs- the task that fails is running under node@13.14.0:

/usr/bin/tar xz --warning=no-unknown-keyword -C /home/runner/work/_temp/7bd7433d-ba81-4237-ac22-1ea42ffa579b -f /home/runner/work/_temp/1e3c4cf6-32b1-405a-90d3-3bb7230d395f
/opt/hostedtoolcache/node/13.14.0/x64/bin/node --version
v13.14.0

And the types that are defined on the package-lock.json for the @types/node entry are: 9.4.5.

Question: would it make sense to add the skipLibCheck flag for this task tsc -p ./tsconfig/compat/tsconfig.cjs.json?

Thanks for the fix @cartant !

Great work, @josepot and @cartant. Thanks for seeing this through!