EventEmitter leak in 2.80

[jabr]

After upgrading to 2.80, I was getting this error in production:

MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 connect listeners added. Use emitter.setMaxListeners() to increase limit

The node.js processes quickly grew to 1-2 GB of resident memory (normally 300 MB) and then CPU load started spiking.

Rolled back to 2.79 and the issue went away.

[mikeal]

It's in one of these:

60f6a00
c7c9431

These are the only commits that add event listeners between those two releases.

[mikeal]

I think I found it 60f6a00#commitcomment-21154407

@FredKSchott @nicjansma do you wanna take a look?

[jabr]

That looks like the issue. The application uses keep-alive connections.

[mikeal]

I'm wondering if the best course of action is to just remove this feature. It doesn't actually work during keep-alive and keep-alive is entirely transparent to the user so the timings object just wont' have this information sometimes and it isn't entirely predictable when they will have it.

[jabr]

Or it could check if the socket is already connected and just set the connect time to the socket time.

[mikeal]

Or it could check if the socket is already connected and just set the connect time to the socket time.

That's probably worse. People will make really bad assumptions about that.

[nicjansma]

See #2566 for an updated PR that works for keep-alive.

Do you think it's sufficient to change the .on to .once?

[mikeal]

@nicjansma no, that won't work.

The problem isn't that it isn't being cleaned up after being called it's that it is probably never called.

[mikeal]

@nicjansma one thing you'll need to consider is that socket level metrics simply aren't applicable in the keep-alive scenario and that being in keep-alive isn't always predictable by your consumers.

[nicjansma]

@mikeal Ah, that makes sense. I can update the handling to ensure the listener is removed, and validate that the socket object isn't kept alive.

Yep, I understand the concerns about the Keep-Alive scenario. In that case, the connect time will be set to the dns time (which will be set to the socket time which will be set to the start time) if any of the phases don't occur. This roughly matches the ResourceTiming processing model.

[mikeal]

you probably do want to use once, but you also probably want to set a property for when connect has already been fired so that you can avoid adding the handler again.

[mikeal]

@nicjansma ping me when the PR gets updated :)

[nicjansma]

@mikeal Absolutely, working on it!

[nicjansma]

@mikeal 6c891da

Tested by running a continuous loop of connection to the same keep-alive address. Before, I saw the above warning within a few requests. Now there is no more warning.

[Huachao]

@mikeal @nicjansma will a newer version be published for this patch?