fix(shareReplay): handle possible memory leaks #5932

josepot · 2020-12-14T10:27:16Z

Description:
Fixes possible memory leaks caused by the shareReplay operator

Related issue (if exists):
#5931

josepot · 2020-12-14T15:46:03Z

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.

josepot · 2020-12-14T16:25:26Z

Actually, sorry, it does fix the memory leak

MaximSagan · 2020-12-15T03:42:06Z

@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

benlesh

These changes don't at all affect the logic of what's going on in this operator, and wouldn't really effect the memory usage.

As I stated in the related issue, it seems the problem is confusion around the default behavior of shareReplay. Which, IMO, has been an ongoing problem.

Hopefully the new operators in version 7 will gain more adoption and we can leave these confusing days behind us.

src/internal/operators/shareReplay.ts

benlesh · 2021-02-10T19:24:27Z

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.

josepot · 2021-02-15T20:42:57Z

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!

cartant · 2021-02-16T10:48:46Z

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

benlesh · 2021-02-16T18:46:14Z

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

benlesh

Add a comment, as suggested, above the new conditional.
Add a test that proves this resolves the issue.

RELATED: we'll need to create a second PR that adds the same test to master.

josepot · 2021-02-16T19:59:39Z

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!

cartant · 2021-02-16T20:20:07Z

Is that possible without testing the internals? 🤔

Yes. I'll add one soon.

cartant · 2021-02-16T22:13:24Z

@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.

cartant · 2021-02-16T22:15:18Z

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

josepot · 2021-02-16T22:30:58Z

@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.

cartant · 2021-02-18T12:41:55Z

@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 · 2021-02-18T12:48:57Z

@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.

josepot · 2021-02-18T13:05:37Z

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 !

benlesh · 2021-02-19T00:59:43Z

spec/operators/shareReplay-spec.ts

@@ -262,4 +262,26 @@ describe('shareReplay operator', () => {
    expectObservable(result).toBe(expected);
  });

+  const FinalizationRegistry = (global as any).FinalizationRegistry;


This is a clever use of FinalizationRegistry.

benlesh · 2021-02-19T01:04:10Z

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

josepot mentioned this pull request Dec 14, 2020

Memory leaks caused by shareReplay when shared observable is synchronous or continuous #5931

Closed

josepot closed this Dec 14, 2020

josepot reopened this Dec 14, 2020

benlesh reviewed Feb 10, 2021

View reviewed changes

src/internal/operators/shareReplay.ts Show resolved Hide resolved

src/internal/operators/shareReplay.ts Show resolved Hide resolved

benlesh closed this Feb 10, 2021

benlesh reopened this Feb 16, 2021

benlesh requested changes Feb 16, 2021

View reviewed changes

josepot force-pushed the fix/shareReplay-mem-leaks branch from 3e64f6b to aff9283 Compare February 16, 2021 19:36

cartant mentioned this pull request Feb 16, 2021

fix: add test from #5932 and plug leak in share #6029

Merged

josepot and others added 3 commits February 18, 2021 13:34

fix(shareReplay): handle possible memory leaks

c792cc3

test: add shareReplay leak test

04bfa70

style(shareReplay): fix linting issues

a3f8984

josepot force-pushed the fix/shareReplay-mem-leaks branch from 86f6342 to a3f8984 Compare February 18, 2021 12:34

chore: fix typescript@latest CI failure

94cf6ef

cartant approved these changes Feb 18, 2021

View reviewed changes

cartant mentioned this pull request Feb 18, 2021

chore: install other @latest packages for ts@latest CI test #6033

Merged

benlesh reviewed Feb 19, 2021

View reviewed changes

benlesh approved these changes Feb 19, 2021

View reviewed changes

benlesh merged commit 6f7d88b into ReactiveX:6.x Feb 19, 2021

This was referenced Mar 6, 2021

Bump rxjs from 6.6.3 to 6.6.6 antonioru/beautiful-react-hooks#216

Closed

Bump rxjs from 6.6.3 to 6.6.6 in /web vmware-archive/octant#2113

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(shareReplay): handle possible memory leaks #5932

fix(shareReplay): handle possible memory leaks #5932

josepot commented Dec 14, 2020

josepot commented Dec 14, 2020 •

edited

josepot commented Dec 14, 2020

MaximSagan commented Dec 15, 2020 •

edited

benlesh left a comment •

edited

benlesh commented Feb 10, 2021

josepot commented Feb 15, 2021 •

edited

cartant commented Feb 16, 2021

benlesh commented Feb 16, 2021

benlesh left a comment

josepot commented Feb 16, 2021 •

edited

cartant commented Feb 16, 2021

cartant commented Feb 16, 2021 •

edited

cartant commented Feb 16, 2021

josepot commented Feb 16, 2021

cartant commented Feb 18, 2021

josepot commented Feb 18, 2021

josepot commented Feb 18, 2021 •

edited

benlesh Feb 19, 2021

benlesh commented Feb 19, 2021

fix(shareReplay): handle possible memory leaks #5932

fix(shareReplay): handle possible memory leaks #5932

Conversation

josepot commented Dec 14, 2020

josepot commented Dec 14, 2020 • edited

josepot commented Dec 14, 2020

MaximSagan commented Dec 15, 2020 • edited

benlesh left a comment • edited

Choose a reason for hiding this comment

benlesh commented Feb 10, 2021

josepot commented Feb 15, 2021 • edited

cartant commented Feb 16, 2021

benlesh commented Feb 16, 2021

benlesh left a comment

Choose a reason for hiding this comment

josepot commented Feb 16, 2021 • edited

cartant commented Feb 16, 2021

cartant commented Feb 16, 2021 • edited

cartant commented Feb 16, 2021

josepot commented Feb 16, 2021

cartant commented Feb 18, 2021

josepot commented Feb 18, 2021

josepot commented Feb 18, 2021 • edited

benlesh Feb 19, 2021

Choose a reason for hiding this comment

benlesh commented Feb 19, 2021

josepot commented Dec 14, 2020 •

edited

MaximSagan commented Dec 15, 2020 •

edited

benlesh left a comment •

edited

josepot commented Feb 15, 2021 •

edited

josepot commented Feb 16, 2021 •

edited

cartant commented Feb 16, 2021 •

edited

josepot commented Feb 18, 2021 •

edited