weyoss / redis-smq

A simple high-performance Redis message queue for Node.js.
MIT License
596 stars 64 forks source link

MaxListenersExceededWarning: Possible EventEmitter memory leak detected #91

Closed hiendaovinh closed 2 years ago

hiendaovinh commented 2 years ago

Hi, I was trying to produce multiple messages and encountered this warning

(node:1055180) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 up listeners added to [Producer]. Use emitter.setMaxListeners() to increase limit
    at _addListener (node:events:601:17)
    at Producer.addListener (node:events:619:10)
    at Producer.once (node:events:663:8)
    at Producer.produce (node_modules/redis-smq/dist/src/lib/producer/producer.js:94:26)

It's from the once(events.UP, proceed).

weyoss commented 2 years ago

Hey @hiendaovinh! Thank you for reporting this issue.

I'm going to see what went wrong ASAP.

weyoss commented 2 years ago

My first though is, this error may occur only in one case: When you create a producer instance, it will try to connect to Redis immediately. At time when your producer instance is not yet fully UP, you tried to publish too many messages. So in this case I expect an exception to be thrown (MaxListenersExceededWarning).

weyoss commented 2 years ago

After a quick analysis, it seems like you tried concurrently from many places, using a single producer instance which has not yet connected to Redis, to publish many messages.

Just to keep things simple and to avoid this kind of errors, maybe producer instances should be manually started using the run() method, the same way as for consumers.

hiendaovinh commented 2 years ago

Yes @weyoss, I was using a single producer to produce multiple messages concurrently. Do you have any recommendation for this kind of pattern? Should I create multiple producers? I got like a thousand of messages to process.

hiendaovinh commented 2 years ago

You were mentioning run() but afaik it's automatically called at constructor. Should I listen for up events before start producing?

hiendaovinh commented 2 years ago

Yeah, I can confirm this will work. I think you should update your documentation.

const producer = new Producer(config)
producer.once(events.UP, () => {
  producer.produce(message, err => {})
  ...
})

Although producers automatically run, it doesn't guarantee that they will be ready. We need to wait for the signal before processing to avoid this kind of warning.

weyoss commented 2 years ago

Do you have any recommendation for this kind of pattern? Should I create multiple producers? I got like a thousand of messages to process.

Producing concurrently many messages, a thousand or a million messages, is totally fine as well as your system can handle it. So the recommended way is to use a single producer instance.

weyoss commented 2 years ago

You were mentioning run() but afaik it's automatically called at constructor.

At this time the run() method is automatically called upon initialization. But I mean as an improvement it shouldn't be that way and this should be changed to manually calling the run() method, like you do for consumers. The point is that the run() method accepts a callback and once the producer is ready and running the callback get called. After that you can do whatever you want with your producer.

weyoss commented 2 years ago
const producer = new Producer(config)
producer.once(events.UP, () => {
  producer.produce(message, err => {})
  ...
})

This is a workaround, it should be fine for now.

hiendaovinh commented 2 years ago

Agree. Thank you for your time!

weyoss commented 2 years ago

It was my pleasure.

Reopening this issue as an improvement is required to be made to avoid this kind of issues.

hiendaovinh commented 2 years ago

I think we should just put a notice to the documentation for awareness and let people decide how to use it properly.

weyoss commented 2 years ago

I think we should just put a notice to the documentation for awareness and let people decide how to use it properly.

Starting with v7.0.6, the proper way is to run your producer instance using the run() method before producing messages.

weyoss commented 2 years ago

See https://github.com/weyoss/redis-smq/blob/master/docs/api/producer.md#producerprototyperun

weyoss commented 2 years ago

Closing as resolved.