Urigo/graphql-modules

Possible memory leak if context keep reference to a promise

manisenkov opened this issue · 9 comments

manisenkov commented

Describe the bug

Last months we looking for an issue in one of our projects that using graphql-modules: they did have a slowly building memory leak that eventually lead the service to crash.

We did investigation and found that issue in the execution context handling. In async hook that handle this process here context is only destroyed with calling a destroy event, but problem is that this event is not called for Promises until they are not collected by GC (as stated in the async_hooks docs).

What happens: execution context somewhere hold a reference to a Promise object and because of that associated context can’t be destroyed because it waits to promise to be GCed, so it creates a circular reference.

We solve it with patching the module code with adding handler for promiseResolve event but it would be great to add such fix to the main package.

To Reproduce
Steps to reproduce the behavior:

Create a store reference to a Promise object in one of the Injectables and observe memory consumption with incoming requests.

Expected behavior

Execution context should be destroyed correctly when an operation is completed.

Environment:

  • OS: macOS 12.3.1
  • @graphql-modules/...: 2.0.0
  • NodeJS: 16.14.1

Additional context

kamilkisiela commented

Do you think you could provide a unit test using weak-napi?

manisenkov commented

Hi @kamilkisiela
I think it should be possible to have a test with weak-napi. I'll work on a PR for the fix on this week.

manisenkov commented

I've tried to use weak-napi but unfortunately attaching any callback to a weak reference makes a test run failed with SIGSEGV, despite all tests are green.
Other idea would be just to look to the size of executionContextStore before and after operation execution. Wdyt?

martyganz commented

It seems we are also running into this issue, have not found a way to solve it for us yet so would be nice if it can be fixed.

@manisenkov do you still have your code anywhere? Maybe I can also have a look and help.

manisenkov commented

@martyganz No, I don't have proper setup code, just played around with weak-napi in tests and it didn't work for me.

So far we just fixed this issue with patch-package and custom patch like this:

--- a/node_modules/graphql-modules/index.js
+++ b/node_modules/graphql-modules/index.js
@@ -188,6 +188,11 @@ const executionContextHook = async_hooks.createHook({
             executionContextStore.delete(asyncId);
         }
     },
+    promiseResolve(asyncId) {
+        if (executionContextStore.has(asyncId)) {
+            executionContextStore.delete(asyncId);
+        }
+    },
 });
 const executionContext = {
     create(picker) {
manisenkov commented

Another option would be to migrate the execution context management to asyncLocalStorage instead of using async hooks.

  • 👍1
kamilkisiela commented

@martyganz @manisenkov
Could you check if 2.0.1-alpha-46bd8e3a.0 solves the issue?

kamilkisiela commented

graphql-modules@2.1.0

manisenkov commented

Hi @kamilkisiela
Sorry for the delay. We have updated the version and removed the patch and see no issues with memory now 👍

  • 🚀1