Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

monaco-graphql: workers not cleaned up -> memory leak #1706

Closed
jensneuse opened this issue Nov 2, 2020 · 9 comments · Fixed by #1994
Closed

monaco-graphql: workers not cleaned up -> memory leak #1706

jensneuse opened this issue Nov 2, 2020 · 9 comments · Fixed by #1994

Comments

@jensneuse
Copy link

I think the behaviour of the monaco language workers leads to memory leaks.
I'm using an extension of the monaco-graphql webpack example with a React application.
I initialize an editor and model on component mount and dispose both on component unmount.

On first mount, looking at the Memory tab of the Chrome devtools I can see 3 workers.
One editor.worker.js and two instances of graphql.worker.js, which seems to be fine.

On first unmount, the editor.worker.js disappears while the two graphql.worker.js stay active.

On second mount I can see a new editor.worker.js instance appear.
On the graphql.worker.js part, I now have 3 active instances.

On the second unmount, the editor.worker.js disappears again, while the 3 graphql.worker.js instances stay.

If I keep re-mounting the component I see the numbers of graphql.worker.js piling up.
It's the same behaviour in Firefox.

I'm just thinking, why do you keep the worker around?
The editor worker disappears immediately once you dismount the editor & model.
There should be no reason to keep them running, does it?
Even if there is a reason to keep instances running, unused instances should still be garbage collected I think?

@acao
Copy link
Member

acao commented Nov 2, 2020

probably an issue with the WorkerManager which should be doing that garbage collection. maybe you can open a PR?

@jensneuse
Copy link
Author

Is there someone I can talk to to get a good understanding of the architecture? There are a lot of moving parts, language server and workers are new to me. If someone could bring me up to speed I'd be happy to submit a PR and fix it.

@acao
Copy link
Member

acao commented Nov 5, 2020

Hopefully i can get a chance in the coming weeks! i'm the person who wrote most of it. @cshaver knows a lot about it as well!

@sachin746
Copy link

sachin746 commented Nov 10, 2020

how to get get started to wok on this issue about where to head to find this issue

@jensneuse
Copy link
Author

use this example
https://github.com/graphql/graphiql/tree/main/examples/monaco-graphql-webpack
then mount and unmount a monaco graphql instance.
You’ll then see the webworker still active and not going away.
You can also emulate this behaviour using React when you mount and unmount the editor by navigating back and forth. For each nav you'll see another JS VM instance in the developer tab with old VM's not being stopped. The VM for the editor itself (monaco code) gets closed immediately as you would expect. The GraphQL workers however don't get stopped.

@acao
Copy link
Member

acao commented Nov 14, 2020

@sachin746 make sure to follow the readme instructions for the monaco example, you need to yarn and yarn build in a few different places first.

@jensneuse this bug is entirely in monaco-graphql's WorkerManager file, so just follow the Development.md instructions and the monaco example instructions, and you'll have a nice workspace for testing monaco-graphql changes manually. the LSP server is not involved at all. though monaco-graphql uses graphql-language-service-interface, that package is not relevant to this bug.

the PR to fix this would only involve changes to that one file. you can see the WorkerManager that monaco-json or monaco-yaml or other monaco modes use as an example, almost exactly the same behavior.

I would set up a call if I had time, but alas I do not currently :(

@jensneuse
Copy link
Author

@acao thanks for the hints. I'll have a look probably next week.

@acao acao added the bug label Feb 1, 2021
@acao
Copy link
Member

acao commented Feb 1, 2021

prioritizing this for the 2.0 effort, added to the roadmap!

@acao
Copy link
Member

acao commented Feb 1, 2021

@jensneuse are you able to provide a codesandbox or some such to reproduce this bug by chance?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants