Azure / azure-event-hubs-node

Node client library for Azure Event Hubs https://azure.microsoft.com/services/event-hubs
MIT License
50 stars 44 forks source link

MaxListenersExceededWarning: Possible EventEmitter memory leak detected #71

Closed yanisIk closed 6 years ago

yanisIk commented 6 years ago

How to reproduce:

Node: 10.0.0 Npm: 5.6.0 tsc: 2.7.2 azure-event-hubs: 0.1.2

Here's the stack trace using node --trace-warnings

(node:15668) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 released listeners added. Use emitter.setMaxListeners() to increase limit
    at _addListener (events.js:244:19)
    at Sender.addListener (events.js:261:10)
    at Promise (C:\...\node_modules\azure-event-hubs\dist\lib\eventHubSender.js:231:30)
    at new Promise (<anonymous>)
    at EventHubSender._trySend (C:\...\node_modules\azure-event-hubs\dist\lib\eventHubSender.js:175:34)
    at EventHubSender.<anonymous> (C:\...\node_modules\azure-event-hubs\dist\lib\eventHubSender.js:68:35)
    at Generator.next (<anonymous>)
    at C:\...\node_modules\azure-event-hubs\dist\lib\eventHubSender.js:9:71
    at new Promise (<anonymous>)
    at __awaiter (C:\...\node_modules\azure-event-hubs\dist\lib\eventHubSender.js:5:12)

FIX: Make sure to await on the client.send() method.

amarzavery commented 6 years ago

Can you share the code snippet? If you are running the sends in a loop, it looks like you did not await on sending the messages.

yanisIk commented 6 years ago

Right now I'm subscribing to a websocket and sending wathever I receive from it without waiting for the promise to resolve (client.send)

What happens when I wait for the client.send() ? Is it waiting for the response from EventHubs or something else ? Because if it's the first case, then it is sending events synchronously and this would be very slow for use cases where you need to send hundreds of events per second.

amarzavery commented 6 years ago

If you are using an async function then you can await inside a for loop. Take a look at an example over here.

amarzavery commented 6 years ago

The promisified send is wrapping the event handler underneath where it adds a listener for every attempt to send and removes it once it gets the accepted, rejected, modified or released event. Since you are not waiting on the promise to complete, too many event handlers are being attached and they are removed at a slower rate. If an error is received that is retryable then it will attempt to retry the send.

yanisIk commented 6 years ago

Yeah you were right, I wasn't waiting on purpose to get the max throughput, now I do and no more warnings. I will edit my issue and include the answer.

But I still have a question: Is the sending process synchronous or async ? Is it waiting for a message to deliver before sending the next one ?

The C# library has two modes: sync and async. But here I don't know if waiting on the client.send() makes it wait on message delivery or not.

This is very important because it can drastically change the throughput.

Thanks !

amarzavery commented 6 years ago

Have you tried client.sendBatch() where you can group multiple messages together (up to 256kb) and they will be all sent together. When you receive them, they would be received as separate messages.

yanisIk commented 6 years ago

No I didn't try batch yet. My use case is very dependent on latency and every millisecond between each event counts. That's why I wanted to send events separately.

amarzavery commented 6 years ago

But I still have a question: Is the sending process synchronous or async ? Is it waiting for a message to deliver before sending the next one ?

It is not a binary answer. I can provide some context. Everything is async in node.js since node.js has it's own event loop. So in that sense the sends are definitely async. I mean there is no need to wait for completion of one send before sending the next one. However since we are wrapping the underlying event based sends into a promise like structure we come across this issue of having more than 11 (I guess that is the default limit of multiple event listeners in node.js) listeners attached to the same object.

The protocol level library that the sdk depends on has an internal buffer for sending messages. If the buffer is full then it stops sending messages until it has room to send more messages. When the messages are sendable the sdk tries to send those messages.

Even after awaiting on sending the message the speed for sending messages should be decent enough. I mean it should be able to send around 100 messages/second.