search

apollographql / graphql-subscriptions

:newspaper: A small module that implements GraphQL subscriptions for Node.js
MIT License
1.59k stars 133 forks source link

withFilter is causing memory leakage because using recursion #212

Closed urossmolnik closed 3 years ago

urossmolnik commented 4 years ago

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

derekgarnett commented 4 years ago

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

alexmcmillan commented 4 years ago

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

urossmolnik commented 4 years ago

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

alexmcmillan commented 4 years ago

@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 commented 4 years ago

@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 commented 3 years ago

@glasser Any news on this?

glasser commented 3 years ago

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? https://github.com/apollographql/apollo-client/pull/7661 (Fine if it's just a top-level test that we have to explicitly link in to CI.)

urossmolnik commented 3 years ago

Hi @glasser, I updated pull request https://github.com/apollographql/graphql-subscriptions/pull/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 commented 3 years ago

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.