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.

Sign up for GitHub

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

Jump to bottom

MaxListenersExceededWarning when using the Cloud logs viewer #4186

Closed
vvagaytsev opened this issue May 8, 2023 · 8 comments
Closed

MaxListenersExceededWarning when using the Cloud logs viewer #4186

vvagaytsev opened this issue May 8, 2023 · 8 comments
Assignees
@thsig
Labels
0.13

Comments

@vvagaytsev
Copy link
Collaborator

Here's the full error:

(node:23396) MaxListenersExceededWarning: (node) warning: possible EventEmitter memory leak detected. 1001 listeners added. Use emitter.setMaxListeners() to increase limit.

This happens when running the Garden serve command and connecting via Cloud.

Might be due to this: https://discord.com/channels/817392104711651328/1102901553544822885
@vvagaytsev vvagaytsev added the 0.13 label May 8, 2023
@vvagaytsev
Copy link
Collaborator Author

@eysi09 is this still reproducible?

@eysi09
Copy link
Collaborator

eysi09 commented May 8, 2023

I still see this, don’t have a proper repro atm

@vvagaytsev
Copy link
Collaborator Author

Ok, thanks! We'll move it to the backlog then.

@eysi09
Copy link
Collaborator

eysi09 commented May 10, 2023

Found the issue and have a repro:

When requesting a command via ws, a bunch of listeners subscribe to the Garden event bus that are never cleaned up.

So simply by using the Clouds logs viewer or command palette, the subscribers pile up until they eventually exceed the limit.

So the solution is to figure out a way to unsubscribe the subscribers that got created during the command run for the requested command, but not others, which may still be needed.

@ShankyJS
Copy link
Contributor

User @ralphv had this issue in Discord when running a task that exports outputs from a DB

https://discord.com/channels/817392104711651328/1106250635931811981/1106250635931811981

cc @eysi09 @vvagaytsev

@ShankyJS
Copy link
Contributor

Seems related to node-fetch/node-fetch#1295

@eysi09
Copy link
Collaborator

eysi09 commented May 13, 2023

Thank @ShankyJS!

And a brief update after some more investigation:

  • The PluginEventBroker does not unsubscribe from events, this is probably the leading cause of "listener overflow" and needs to be addressed.
  • Commands executed via websocket requests also don't clean up after themselves but the volume is a lot lower and this will most likely resolve itself when we introduce "instance manager". So I don't think we need to worry about that for now.

Re-assigning to @thsig who's been looking into the former cause.

@eysi09 eysi09 assigned thsig and unassigned eysi09 May 13, 2023
thsig added a commit that referenced this issue May 13, 2023
@thsig
fix(core): improve event listener cleanup
7e78f7b 
Closes #4186.

Before this fix, we weren't removing event listeners registered by the
plugin event brokers (mostly for the `_exit` and `_restart` control
events).

This is now done in a simple way via the new `onKey`  and `clearKey`
methods on the event bus, which facilitates removing all listeners
matching a given key.

We use `garden.sessionId` as the key now, which is not quite ideal
when running concurrent commands in `garden dev`, but we'll be assigning
a command-unique `sessionId` in an upcoming PR (which gives us precisely
the semantics we want here).
@thsig thsig mentioned this issue May 13, 2023
Merged
thsig added a commit that referenced this issue May 13, 2023
@thsig
fix(core): improve event listener cleanup
c95e3d5 
Closes #4186.

Before this fix, we weren't removing event listeners registered by the
plugin event brokers (mostly for the `_exit` and `_restart` control
events).

This is now done in a simple way via the new `onKey`  and `clearKey`
methods on the event bus, which facilitates removing all listeners
matching a given key.

We use `garden.sessionId` as the key now, which is not quite ideal
when running concurrent commands in `garden dev`, but we'll be assigning
a command-unique `sessionId` in an upcoming PR (which gives us precisely
the semantics we want here).
@thsig
Copy link
Collaborator

thsig commented May 13, 2023

Closed by #4258.

@thsig thsig closed this as completed May 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@thsig thsig

Labels
0.13
Projects
No open projects
0.13
Status: Done
Milestone
No milestone
Development

No branches or pull requests
4 participants
@thsig @eysi09 @vvagaytsev @ShankyJS