Should remove listener to timeout event

[nkbt]

At this moment with default settings there is a subscription added

req.on('timeout', onTimeout(delay, next))

It would be a good idea to unsubscribe from it to avoid mem leak in a long run.

Could be something like:

const onTimeoutCb = delay => {
  req.removeListener('timeout', onTimeoutCb);
  onTimeout(delay, next)();
};
req.on('timeout', onTimeout);

[dougwilson]

Due to how the gc works there shouldn't be a memory leak. Can you provide an app that demonstrates a memory leak so I can replicate the leak issue? This way we'll also know that we fixed the issue across all supported Node.js versions we work on.

It doesn't need to be your real code, just code I can copy and paste and run to see the leak. I can then point autocannon at the server code you provide and watch the v8 memory output to monitor the leak and object dump to see what is getting leaked 👍

[nkbt]

Due to how the gc works there shouldn't be a memory leak

What do you mean by that? I am pretty sure when I subscribe to events and not unsubscribe afterwards, that callback will be hanging around forever.
I reckon there could be potentially some internal req destroy that will unsub from all events when request completes, but I haven't check nodejs source for this yet.
Maybe I'm missing something?

In my internal test (on actual app) as soon as I switched respond: false and implemented my own solution that looked very much like

const timeout = (req, res, next) => connectTimeout(req, res, () => {
  const onTimeout = delay => {
    req.removeListener('timeout', onTimeout);
    next(new TimeoutError())
  };
  req.on('timeout', onTimeout);
  next();
})

After 200k requests container was using 150mb of mem.
With const timeout = connectTimeout after same 200k requests it was about 175mb.
Note, that I was testing it inside fresh docker container each time, so env is absolutely clear at the start and there was nothing else happening between runs.

I may profile it a bit more on Monday, but as a rule of thumb I'd always unsub from any subscription

PS: it is not that easy to test since request is quite small. Though I guess if add there some very large string var in closure it would be way more obvious.

[dougwilson]

It doesn't matter how big or small it is: that is why I will take a memory dump, because you'll be able to see a leak of any size easily since the number of remaining objects will be the same regardless of their size 👍

[nkbt]

Probably if we check connectTimeout in isolation without express and stuff and then test - it would be more obvious. Because when I was checking mem dumps it had quite a few diffs =( and I did not have enough time to investigate it further. I found interesting that no matter how many requests I made and if I ran gc manually all the time, there always was last (exactly last) route leftover in diffs (strings)

[dougwilson]

Haha, yea, they can be tedious. I've looked through so many I have some shell scripts to help me expediate some tasks, which is why I'm happy to volunteer to figure it out 👍

[nkbt]

Actually req.on can be just replaced with req.once('timeout')
https://nodejs.org/api/events.html#events_emitter_once_eventname_listener

[dougwilson]

So I'm going to close this since it seems to have stalled and never got the needed information here to proceed.