Fix memory leak in withFilter cause by recursion

[urossmolnik]

withFilter function is causing memory leak because of Promise spec. Related: nodejs/node#6673.

This is a big problem when you have long living subscriber to subscription who skips most of messages (all Promises in recursion chain are kept in memory).

How to reproduce:

Start a subscriber to a subscription which rejects messages and start publishing messages. You'll notice memory keeps increasing. Memory profile will confirm.
Do the same thing using refactored withFilter and you'll notice memory is not increasing.

Pull Request Labels

• has-reproduction
• feature
• blocking
• good first review

Fixes #212

[apollo-cla]

@urossmolnik: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[nurettintopal]

any updates on this PR?

[vincentdesmares]

I also have memory leaks using this package. It would be nice if this PR could be merged.

[groundmuffin]

same here

[dylancruselutd]

Would also like to see this merged.

[skarahoda]

➕

[nurettintopal]

@urossmolnik: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@apollo-cla what do you think about this PR? This PR has been waiting for 5 months. please write a feedback about this PR.

[urossmolnik]

@nurettintopal I signed agreement.

[OmgImAlexis]

The typescript errors still need to be fixed.

node_modules/@types/chai/index.d.ts:121:9 - error TS8020: JSDoc types can only be used inside documentation comments.
121         any?, // actual value
            ~~~~
node_modules/@types/chai/index.d.ts:122:9 - error TS8020: JSDoc types can only be used inside documentation comments.
122         boolean? // showDiff
            ~~~~~~~~
node_modules/@types/chai/index.d.ts:126:16 - error TS2370: A rest parameter must be of an array type.
126         assert(...args: AssertionArgs): void;
                   ~~~~~~~~~~~~~~~~~~~~~~
src/test/asyncIteratorSubscription.ts:136:52 - error TS2345: Argument of type '(results: AsyncIterator<ExecutionResult<ExecutionResultDataDefault>>) => void' is not assignable to parameter of type '(value: ExecutionResult<ExecutionResultDataDefault> | AsyncIterableIterator<ExecutionResult<Execu...'.
  Types of parameters 'results' and 'value' are incompatible.
    Type 'ExecutionResult<ExecutionResultDataDefault> | AsyncIterableIterator<ExecutionResult<ExecutionResu...' is not assignable to type 'AsyncIterator<ExecutionResult<ExecutionResultDataDefault>>'.
      Type 'ExecutionResult<ExecutionResultDataDefault>' is not assignable to type 'AsyncIterator<ExecutionResult<ExecutionResultDataDefault>>'.
        Property 'next' is missing in type 'ExecutionResult<ExecutionResultDataDefault>'.
136     Promise.resolve(subscribe(schema, query)).then((results: AsyncIterator<ExecutionResult>) => {
                                                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[urossmolnik]

@OmgImAlexis these are not related to this pull request. Should I fix them as part of it?

[OmgImAlexis]

Maybe in another PR? This would need to be merged after that though.

[paumartin]

Hi! Any updates on this issue?

[urossmolnik]

@OmgImAlexis sorry missed this. I just pushed changes to this pull request. Hope this is ok.

[wyattjoh]

Also seeing this issue after investigating many of our nodes running this code were experiencing memory leaks. Heap snapshots revealed lots of memory was being eaten from this leaked variable.

[OmgImAlexis]

@grantwwu any chance on this being merged soon?

[grantwwu]

I am no longer working on GraphQL, sorry.

Please try to contact someone from Apollo for maintenance of this repo.

[OmgImAlexis]

Could you please tag in the right person/team? I’d tag a team but I couldn’t see anything come up when I tried looking for them.

[grantwwu]

I don't/didn't work at Apollo. Your best bet for contacting them might be the Apollo Spectrum https://spectrum.chat/apollo?tab=posts. I don't know who on their side is supposed to be interacting with the open source community.

When I was still working on GraphQL at my last job, after initially having some success working with Apollo devs to make improvements, I found it harder and harder to reach out to them to get things reviewed or to get help with how graphql-subscriptions interacted with other components of the GraphQL ecosystem. (I actually still have an unmerged bugfix PR #205.) That, and the fact that I am not using GraphQL at all in my job, is why I stopped spending time on this.

[niklaskorz]

Well, @hwillson is the most recent Apollo team member to push to this repo, so hopefully this is the right person to ping

[OmgImAlexis]

@glasser what're the odds in getting this merged after the build is fixed?

[glasser]

I plan to review it soon, but since I'm not super familiar with this package and memory leaks are subtle, I know it will take at least a bit of care. I'm about to take a week of vacation so don't expect any activity before the 22nd at the earliest!

[brettjashford]

this is a visualization of running the same load test before the suggested change in this PR and then after. in the asynchronous resources chart, notice the long tail in the first group (before the change in this PR). this is because each filtered event results in an unresolved promise that is kept in memory. these accumulate over time and can trigger crashes/restarts with high throughput.

shoutout to @urossmolnik @OmgImAlexis for the fix 🙏

[glasser]

I have to say: I don't quite understand precisely why the original code causes the issue or how the new code avoids the issue.

But the new code does appear to have the same high-level semantics as the old code, and the test is VERY convincing. So I'll merge and release.

[glasser]

Released in 1.2.1.