josdejong / workerpool

Offload tasks to a pool of workers on node.js and in the browser
Apache License 2.0
2.06k stars 147 forks source link

MaxListenersExceededWarning with worker_threads in node.js #373

Closed Michsior14 closed 1 year ago

Michsior14 commented 1 year ago

Currently there is no way to set max listeners (10 by default) for worker_thread, thus if you have high load across more than 10 threads the warning will popup.

I am interested in fixing this upstream either by setting the listeners value to higher value (e.g. 100) by default or providing the new option for WorkerHandler. What would be your preference?

josdejong commented 1 year ago

Thanks. Some questions/thoughts:

  1. Is there a downside to just setting a higher value like 100?
  2. Is this something users will want to fiddle with?
  3. Can we automatically set max listeners according to the configured maxWorkers option?
Michsior14 commented 1 year ago
  1. Could be harder to detect potential memory leak, although I saw similar fixes for worker implementations.
  2. Possibly yes, in some cases it can narrow down the place where leak could occur.
  3. Maybe, I would need to check a bit more the implementation to make sure it's feasible (e.g. are the listeners cleared/shared across the worker calls).
josdejong commented 1 year ago

Ok so it makes sense to have the setting configurable, like for debugging memory leaks. It would be nice if we can reuse maxWorkers for this I think, please let me know if you think that will work.

Michsior14 commented 1 year ago

Uhhh, it turned out that it's not workerpool fault and everything works as expected here (only one listener for parent and one for child thread) . The issue was with sentry initialization during job execution. Sorry!

josdejong commented 1 year ago

👍