andywer / threads.js

🧵 Make web workers & worker threads as simple as a function call.
https://threads.js.org/
MIT License
3.04k stars 161 forks source link

MaxListenersExceededWarning when subscribing to worker’s observable: how to get an EventEmitter and setMaxListeners() on it? #312

Open strogonoff opened 3 years ago

strogonoff commented 3 years ago

I’m tracking status of certain entities, on which worker operates, this way:

In the worker, I keep one global dictionary with a Subject instance per each entity, and when operations happen on that entity I call entities.next(newStatus).

In host (Electorn’s main), I list all entities to keep track of, and run worker.streamEntityStatus(entityID).subscribe(doSometingWithStatus) once for each in the beginning. (Status is then relayed to the renderer, etc.)

Once the user adds the 11th entity, though, there’s that warning:

(node:5294) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 message listeners added to [Worker]. Use emitter.setMaxListeners() to increase limit

The docs say:

This limit can be changed for individual EventEmitter instances using the emitter.setMaxListeners(n) method.

I suppose observable hides an EventEmitter instance somewhere, and it could be possible to get to it and setMaxListeners on it according to the number of entities I am tracking?

andywer commented 3 years ago

Hmm, that's weird. threads.js subscribes to messages that the main thread receives from the worker via a .addEventListener("message", handler) call.

What's weird is that it normally only subscribes once to each worker. So either this is a bug that for some reason I never ran into before and no one else reported either or you call spawn() several times on the same worker, for instance. Both seem quite unlikely…

So you are running threads.js in node, right? Can you state your node version and threads.js version?

Patbox commented 3 years ago

I can confirm this issue. Node v12.16.1 (+ ts-node/ still happens without it), threads.js 1.6.3

image

All my worker usage: image image

After adding to impl.node.js image It's registered every use of functions image

It happends every time, any function is used

Patbox commented 3 years ago

Okay, if you need something to test it, here's my app/server thing https://github.com/VoxelSrv/voxelsrv-server/tree/tests/threads.js-memory-leak

It's a server for my game, so you will need to connect via http://voxelsrv-master.pb4.eu?server=localhost:3000

andywer commented 3 years ago

Thanks a lot, @Patbox!

Still so weird, though… It happens while only one spawning once, it seems. And I can run all the tests with node 12.6.3 and get no such warning, for instance.

Can you quickly try to turn the console.log() into a console.trace() and post the stack trace?

Patbox commented 3 years ago

image I think the warning is caused by function being called something like 20 times per second (I'm using it for world generation).

andywer commented 3 years ago

Ahh, I see the issue now. You run into this message if you call a worker thread in rapid succession.

The issue

Every call to the worker thread leads to a new message listener being added to the worker. As the message listener is also unsubscribed again once the call is done, this will only lead to the warning if you call the same worker rapidly without waiting for the previous calls to finish.

The solution

We should fix that by using event delegation or explicitly increasing the warning limit by a lot for this particular event emitter.

Your code & concurrency

It is also shines a light on what might be an issue with your code, performance-wise: You should probably use a thread pool. I think that calling the worker threads in rapid succession without waiting for previous calls to be done might have a negative impact on performance as you "drown" the workers in work, instead of dispatching jobs to them at a controlled pace.

I might be wrong, though. Would be super interesting to see a benchmark of your code when using a thread pools and without. When using a thread pool it might also make a major difference to manually set a higher concurrency limit for the pool (like 1x, 2x or 4x the number of CPU cores).

morozig commented 3 years ago

I have same warning using nodejs and pool with high concurrency. As @andywer explained it is not a problem, but here is a simple showcase: workercode workerwarning

Christilut commented 2 years ago

Any way we can hide this warning? Seems to always be 11 listeners so let's just put the limit on 12?

flipswitchingmonkey commented 1 year ago

I'm also getting this warning now all the time (in my case the worker is a log writer and it is often getting hammered with calls - works fine, but throws this warning regularly now :( ) Can we change the setMaxListeners setting somehow?

Mrazator commented 1 year ago
  1. I have one worker thread called in rapid succession with high concurrency and I am getting the same warning.

Can we have the max number of listeners configurable? Setting 100 limit still won't be enough.

  1. Compared to the pool of one thread with high concurrency, there's most likely some overhead, because single thread is still much more performant.

It is also shines a light on what might be an issue with your code, performance-wise: You should probably use a thread pool. I think that calling the worker threads in rapid succession without waiting for previous calls to be done might have a negative impact on performance as you "drown" the workers in work, instead of dispatching jobs to them at a controlled pace. I might be wrong, though. Would be super interesting to see a benchmark of your code when using a thread pools and without. When using a thread pool it might also make a major difference to manually set a higher concurrency limit for the pool (like 1x, 2x or 4x the number of CPU cores).

  1. Compared to the pool of one thread, with low concurrency (1, 2, 4, 8), both single thread pool above and simple single thread are still more performant.

  2. Compared to the pool of 7 threads (os.cpu() -1), with low concurrency (1, 2, 4, 8), all of the above are more performant.

Actually #1 is the only solution which would be worth for switching to worker_threads, since #2 is just a little bit faster than having everything on the main thread (not worth the switch) and #3 & #4 are just much slower.