actionhero / node-resque

Node.js Background jobs backed by redis.
https://node-resque.actionherojs.com
Apache License 2.0
1.37k stars 151 forks source link

Worker did not remove the event listener when it ended. #803

Closed peteAhn closed 1 year ago

peteAhn commented 2 years ago

Hi, I've found the following warning message.

(node:80974) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 21 end listeners added to [Redis]. 
Use emitter.setMaxListeners() to increase limit

And then investigated it. As a result, First, the multi-worker had been running one or two if the queue is empty. Second, Each worker added the event listener into the Redis when the worker started. Finally, any worker did not remove the event listener when it ended.

As the time goes the event listener adds infinitely. I think when the worker ends - in the end method of the worker, it should remove the event listener in the Redis.

Please let me know your opinion. Thanks.

evantahler commented 2 years ago

We should be doing most of that, e.g. https://github.com/actionhero/node-resque/blob/main/src/core/multiWorker.ts#L275-L277. There might be bugs! Can you share a way to reproduce that problem?

peteAhn commented 2 years ago

We should be doing most of that, e.g. https://github.com/actionhero/node-resque/blob/main/src/core/multiWorker.ts#L275-L277. There might be bugs! Can you share a way to reproduce that problem?

Hi, evantahler.

The worker has the queueObject and it is connected. https://github.com/actionhero/node-resque/blob/5092dc114ccbfe1de79ae02f7ee5bfb207561473/src/core/worker.ts#L141

At this time, In the connection class, it is registered the event listener on the Redis. https://github.com/actionhero/node-resque/blob/5092dc114ccbfe1de79ae02f7ee5bfb207561473/src/core/connection.ts#L78

However, in the end method of the worker class, the connection is ended. https://github.com/actionhero/node-resque/blob/5092dc114ccbfe1de79ae02f7ee5bfb207561473/src/core/worker.ts#L190

But also, In the connection class, it is not deregistered the event listener on the Redis. So then whenever the worker connects it, the event listener adds infinitely. https://github.com/actionhero/node-resque/blob/5092dc114ccbfe1de79ae02f7ee5bfb207561473/src/core/connection.ts#L144

I hope that makes sense. Please feel free if you have any further questions.

Thanks.

bbyong2410 commented 2 years ago

We should be doing most of that, e.g. https://github.com/actionhero/node-resque/blob/main/src/core/multiWorker.ts#L275-L277. There might be bugs! Can you share a way to reproduce that problem?

Each workers add the 'error', 'end' event listeners to Redis at worker start. When the worker end, the event listeners added to the worker self are removed, but the listeners added to Redis are not removed.

Here is example for reproduce this problem:

import { Connection, MultiWorker, Queue } from 'node-resque'

const connectionDetails = {
  pkg: 'ioredis',
  host: '[127.0.0.1](http://127.0.0.1/)',
  password: null,
  port: 6379
}

const connection = new Connection(connectionDetails)

await connection.connect()
console.log('connection connected:', connection.redis.listenerCount('error'), 'error listeners added to [Redis]')

const queue = new Queue({ connection: connection }, {})
queue.connect()
console.log('queue connected:', connection.redis.listenerCount('error'), 'error listeners added to [Redis]')

const multiWorker = new MultiWorker({ connection: connection, queues: [] }, {})

multiWorker.on('start', (workerId) => {
  console.log('worker[' + workerId + '] started:', connection.redis.listenerCount('error'), 'error listeners added to [Redis]')
})
multiWorker.on('end', (workerId) => {
  console.log('worker[' + workerId + '] ended:', connection.redis.listenerCount('error'), 'error listeners added to [Redis]')
})

multiWorker.start()

/*
result:
connection connected: 1 error listeners added to [Redis]
queue connected: 2 error listeners added to [Redis]
worker[1] started: 3 error listeners added to [Redis]
worker[2] started: 4 error listeners added to [Redis]
worker[2] ended: 4 error listeners added to [Redis]
worker[2] started: 5 error listeners added to [Redis]
worker[2] ended: 5 error listeners added to [Redis]
worker[2] started: 6 error listeners added to [Redis]
worker[2] ended: 6 error listeners added to [Redis]
worker[2] started: 7 error listeners added to [Redis]
worker[2] ended: 7 error listeners added to [Redis]
... ...
*/
evantahler commented 2 years ago

Amazing - please submit a PR with the fix, and I'll get a release out quickly!

herval commented 2 years ago

Hi! Came here to report this problem. Are there any plans to still merge this?

evantahler commented 2 years ago

A PR with a fix was never submitted. Please let me know if you need help working on this!

herval commented 2 years ago

I'm not sure I have enough familiarity w/ the codebase to contribute atm :-(