fix: Memory leak on client.js 'connected' event #348

[robertsLando]

No description provided.

[mcollina]

This needs a unit test

[mcollina]

This needs a test of the original "memory leak warning" that it claims to fix.

[mcollina]

The code is ok, the test on the EventEmitter warning is missing

[robertsLando]

@mcollina What do you mean?

This ?

process.on('warning', t.fail);

[robertsLando]

Without the queue fix the test fails so it tests also EventEmitter warning now

[mcollina]

lgtm

[robertsLando]

@mcollina Can I merge this or need to wait reviwers?

[mcollina]

please wait for a bit

[gnought]

Why we need to queue all packets between CONNECT and connected event?
We emit the "connected" event at

aedes/lib/handlers/connect.js

Line 93 in f62a343

this.client.emit('connected')

. After we receive CONNECT and trigger connect handler, we still have do authentication, what will happen for the queue packets if the authentication fails?

[robertsLando]

@gnought connected event is trigger after connectActions

We need to queue packets because like you can see in issue #348 after the first connect packet is received all other packet init a listener to connect event here and this throws the eventemitter warning

[gnought]

The memory leakage occurs obviously we are keeping add listener in a same 'connected' event, and over the default limit 10 in https://nodejs.org/docs/latest-v12.x/api/events.html#events_emitter_setmaxlisteners_n

Currently we hook packet event on https://github.com/moscajs/aedes/blob/master/lib/client.js#L51, we parse all pending packets once 'connected' is triggered.

I think we should drop all packets before there is a clientReady or client event occur
That means in https://github.com/moscajs/aedes/blob/master/lib/client.js#L333-L335, we should change to

    if (client.connected)
      handle(client, packet, client._nextBatch)

[robertsLando]

@gnought @mcollina Dropping packets could be an option but I don't know if this is spec compilant. Should we reject packets before the client is connected successfully? Should client wait to receive the connack before sending other commands? If so we can drop packets

[gnought]

Rejecting packets before "connected" event makes sense. We could get rid of unauthenticated packets. Clients should see if the connection is ready like ioredis ready event.
Users may make use of preConnect to implement their protection logic, otherwise we need to think a way how to clear those queued packets if it is unauthenticated client.

[gnought]

Yes, we can, but the fix does not convince me.
If it is under attack and there are lots of delays in preConnect per client with a high volume traffic. All queues are growing bigger and consumes a lot of memory, and we will let Node to use extra power to cleanup all items, even there may be a message lag if Node does not process faster enough.

[mcollina]

I think we can error the client after 42 messages being queued, making it configurable by the user.

[GavinDmello]

I think we can error the client after 42 messages being queued, making it configurable by the user.

@mcollina Life, universe and everything ?

I'm 👍 for configurable amount of messages.

[robertsLando]

@mcollina Should be ok now?

[gnought]

It sounds like that we still queue messages after connected.
What happen when the incoming messages are faster than the processing rate?

[robertsLando]

It sounds like that we still queue messages after connected.

After connected event all queued packets are processed and the queue is set to null

What happen when the incoming messages are faster than the processing rate?

I think that this problem would be the same with or without the initial queue, if the incoming messages are faster then the processing rate simply we get a stack overflow but this would not be caused by the queue

[robertsLando]

@mcollina So what about this?

[mcollina]

Code looks good, I've left a few nits to fix.

[mcollina]

code is ok, can you revert the linting changes to the typescript file?

[robertsLando]

@mcollina done

[mcollina]

lgtm

If you’d like to change linting, add a PR with a linter for the ts files.

[robertsLando]

@mcollina I don't know which ide you are using, if everyone is using vscode we could add .vscode folder with all the formatters used (maybe another PR for this)

[mcollina]

I prefer integrations that are validated by CI, not by the editor.

[mcollina]

(I use vim)

[mcollina]

this conflicts with master now

[robertsLando]

@mcollina It doesn't really conflicts it just fails the coverall tests as now lib/handlers/connect.js has reduced coverage https://coveralls.io/builds/28372782

What should I do?

[robertsLando]

The coverage is decreesed by 0.04% causing that red cross on ci but all other tests are good. Should we create a separate job for coverage?

[mcollina]

increase unit test coverage Il 29 gen 2020, 08:10 +0100, Daniel Lando <[email protected]>, ha scritto:

…

The coverage is decreesed by 0.04% causing that red cross on ci bu all other tests are good. Should be create a separate job for coverage? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.