Memory leak in Client about basic-ftp HOT 13 CLOSED

Thanks for the ticket. I had a quick look using the allocation sampler of the memory profiler but the only thing that I see right away is some Node internals. See screenshot, blue bars represent data not collected by GC yet. I've run this without your code-based memory usage tracking and logging.

It's not immediately obvious to me what this is and whether its something the library is responsible for. Could you have a look on your end? I can see entries related to timers but nothing that the library itself does. It doesn't use any setTimeout for example but uses internal timeout tracking of Socket instances.

I've used node --inspect myTest.js, then opened Chrome with chrome://inspect, selected the remote target myTest.js. Then in the Memory tab I've chosen "Allocation Sampling" and let it run for a bit.

from basic-ftp.

I made 2 heapsnapshots, one at iteration 102 of the while loop and the other at iteration 224. Given the graph above the garbage collector must have run multiple times in between. I compared the snapshots and see that 122 Socket objects are retained:

I don't understand the Retainers overview yet, but I do see timeout being listed. I can't pinpoint the problem yet, but tried already with setTimeout(0) without any difference.

from basic-ftp.

Are you sure that isn't just the total number of sockets ever allocated?

from basic-ftp.

Your screenshot shows a "summary", not a "comparison" between snapshots as the screenshot here below. See the summary/comparison dropdown at the top left. I haven't run as many iterations as you do so, and I'm absolutely open to the possibility of a memory leak. But I haven't seen anything specific yet that originates from the library.

from basic-ftp.

I'm off for a week, but will investigate further after that.

Ps. My screenshot indeed shows the summary, I selected only objects allocated between both snapshots (see a bit to the right). I would say that it cannot know about objects that are removed in between, as these are snapshots, not captures of the whole time window.

from basic-ftp.

Yes, to see garbage collection in action you need to use the allocation sampler of my first reply or use the comparison tool to compare single snapshots to see what was removed (see columns referring to deleted, freed and deltas).

In any case, thanks for your help!

from basic-ftp.

I've now also used --inspect and chose "Allocation instrumentation on timeline" as it states "Use this profile type to isolate memory leaks" and I checked "Record stack traces of allocations". Then I see similar stuff as in my screenshot above, ao. a lot of Socket objects (and FTPContext objects):

from basic-ftp.

Also chiming in to help out.

I did a little bit of digging and this is what I found about net.Socket: https://stackoverflow.com/a/33871679/5688047

Nodejs caches the Socket objects to be used in future.

Perhaps that is what is happening? I need to dive in a bit more to understand what is happening.

from basic-ftp.

In FtpContext.ts in _closeSocket I found that socket.destroy() on line 395 doesn't happen. I tried to add socket.destroy() just below socket.end() on line 397 and this results in a memory profile without Socket objects.

Why was that timeout needed in the first place?

from basic-ftp.

I tested with node 18, 20 and 21 to see if the Sockets are cleaned up in newer versions of node, however that doesn't happen. So my solution is to not use the timeout but just to destroy the socket right away.

from basic-ftp.

I'm okay with merging the suggested PR. The whole thing with the timeout was probably me way overthinking something. @everhardt any last words? If you try the suggested PR do you still see the issues on your end?

from basic-ftp.

I can confirm that the memory leak is gone with the suggested PR. Thanks @martijnimhoff and @patrickjuchli!

from basic-ftp.

Fix is published in 5.0.5

from basic-ftp.