withFilter is causing memory leakage because using 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.

Solved by #209

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

[derekgarnett]

Also experiencing this big time. Using the new withFilter code from the pull request seems to fix it.

[alexmcmillan]

Also suffering disproportionately due to this issue; is there anything I can do to help this fix be merged?

[urossmolnik]

@alexmcmillan get attention from repository contributors :)
Pull request has failing tests but those are not introduced with changes.

[alexmcmillan]

@urossmolnik so the PR is otherwise acceptable?

Damn. Just noticed we're coming up to 4 months since the last commit to this repo... are there any alternatives that are being maintained?

[urossmolnik]

@alexmcmillan I think so.

I have another branch on my fork with fixed tests and post-install script so you can install this package directly from github. If you want to test that just point graphql-subscriptions package to
github:urossmolnik/graphql-subscriptions#v1.2.0.

[kundkingan]

@glasser Any news on this?

[glasser]

Hi @kundkingan. I'm newly responsible for maintaining Apollo Server but I'm not sure if the scope of that work is going to include graphql-subscriptions. I did just publish a new version of this package but that was just publishing some long-merged but unreleased changes that allows Apollo Server to be installed with newer dependencies.

Is it possible to write a test for the proposed fix PR? That would make it easier to merge as somebody not super familiar with this codebase. Maybe something like this Apollo Client test using FinalizationRegistry? apollographql/apollo-client#7661 (Fine if it's just a top-level test that we have to explicitly link in to CI.)

[urossmolnik]

Hi @glasser, I updated pull request #209 and added a test covering memory leak.
If you replace with-filter implementation with old one and run this test, you can see it fails quite badly.

[glasser]

Fix released in 1.2.1. Thanks to @urossmolnik for your patience and an excellent test that allowed me to comfortably merge the PR despite not quite understanding how it achieved its goals.