Memory leak

[MaximBG]

I suggest to add this.currentNode = undefined; inside the Consumer's _destroy() method. Killed Consumer still holds the whole linked list of messages.

[wilensky]

UP! @jondubois

Experiencing same issue here. Last stream message always remains in memory even after _destroy(). On intensive scale this causes increased memory consumption.

Tested on v4.1.0 - still there

Maybe there is a sacred stuff behind keeping last message in stream :question: :smiley:

[jondubois]

@MaximBG Sorry I missed this issue earlier.

@wilensky can you reproduce it with a short snippet of code using WritableConsumableStream?

Note that to ensure that the Consumer is properly garbage-collected, you must ensure one of the following two things are carried out:

1. Call either stream.close() or stream.kill() to end/cleanup the entire stream and all consumers and loops bound to it. Note that stream.close() will finish iterating over pending messages first so it will only clean up after a delay depending on how many messages are pending in the queue.

2. If the stream is not ended explicitly as shown above, make sure that all the relevant loops and consumers terminate on their own. There are 3 things you need to check to ensure this:

• Ensure that all of the loops in the form for await (let data of stream) { ... } are ended (e.g. using a break statement inside the loop).
• Ensure that every consumer which you created with stream.createConsumer() is consumed using a for-await-of loop and that the loop eventually ends (e.g. using a break statement inside the loop).
• Otherwise, if the consumer was created with stream.createConsumer() but never iterated over using a for-await-of loop, then you need to explicitly destroy it using consumer.return() - This method has a line delete this.currentNode which does essentially what @MaximBG suggested. See the test case here for usage example: https://github.com/SocketCluster/writable-consumable-stream/blob/master/test/test.js#L923

Note that the return() method is a special JavaScript method which gets called automatically when you break out of a for-of/for-await-of loop but if you create the iterable without ever iterating over it, then it will not get called so in that case it needs to be invoked explicitly.

Note that using stream.kill() is the simplest and surest way to terminate and clean up a stream and all its consumers. This method should cause all consumers to break out of their for-of loops and also to remove the reference to themselves from the stream so assuming that there are no other references being held in your code, they should be garbage collected.

Based on this information, let me know if the issue persists. A snippet would help.

[wilensky]

@jondubois thank you for quick reply! We will carry out additional investigation on this point in nearest iteration and I will get back with smth. reasonable later on.

[jondubois]

Cheers. I'll be keeping an eye on this. Please keep me updated.

Also, if you're using SocketCluster, disconnecting the socket should clean up all the consumers but let me know if you found a situation where this is not occurring which is not related to your code.

[wilensky]

Yes, we observed this in terms of SocketCluster (v19 btw.), what I forgot to mention.

We see that disconnected sockets remain in memory and that lead us to WriteableStreamConsumer, so it seems that the last message in stream remains (it references a socket) and smth. prevents proper GC and that accumulates. We will finish ongoing code review in this regard and see whether issue is still reproducible, so we can exclude possible code inconsistencies.

[jondubois]

Feel free to post a short snippet with SocketCluster server if you can reproduce. Also, ensure that you don't have any setTimeout() or setInterval() which might be holding onto sockets or loops inside the main 'connection' loop:

(async () => {
  for await (let { socket } of agServer.listener('connection')) {
    // Handle socket connection.
  }
})();

If the socket is disconnected, but it (or a loop which consumes it) is still referenced somewhere (e.g. inside a function closure), it could leak memory so it's important to make sure all references are removed in your code.

[jondubois]

I tried reproducing the issue with socketcluster-server v19.0.1 on Node.js v14.20.0 and v18.10.0.

I wasn't able to reproduce the issue with my setup but I noticed that with Node.js v18.10.0, it takes much longer to garbage collect certain objects. Some of the objects created early on were not GC'd until much later. Whereas Node.js v14 GC'd objects much more regularly.

I only tested locally and basically refreshed the page to create more sockets so this may not be as complex as your situation.

[MaximBG]

I just want to clarify my case, which is relevant for v.17 I hold list of Consumers and at some point I kill them. It turned out, that killed consumer still holds a node of the linked list of the messages and thus prevents memory release. I fixed the issue by releasing the list of killed Consumers, but I think that it is a good idea to implement my suggestion since as I see other people encounter similar problems.