Broker: Memory leak when sockets never disconnect about live-mutex HOT 8 CLOSED

I used V8's built-in inspector.
You just have to run your NodeJS process using --inspect flag, which exposes debug functionalities on a specific websocket.
Chrome is then used as a client for convenience, as it is well integrated (since NodeJS uses the same V8 engine developed originally for Chrome).
You can do some debug, memory inspection, etc by connecting to the exposed websocket visiting URL: chrome://inspect/#devices

You can get a clearer explanation on the official documentation:
https://nodejs.org/de/docs/guides/debugging-getting-started/

Here it is a more pratical guide:
https://medium.com/tech-tajawal/memory-leaks-in-nodejs-quick-overview-988c23b24dba

I recommend clicking on Open dedicated DevTools for Node instead of inspect on Remote Target section, since it also enables more advanced debugging that can be useful, with breakpoints, variable preview on current scope, etc.

from live-mutex.

Good catch, I have tested the broker for a growing memory footprint in the past, but not recently, will take a look, thanks. That map used to be used, but no longer is at the moment, I will most likely just comment it out, and publish a new version.

from live-mutex.

so it is being used here:

  cleanupConnection(ws: LMXSocket) {
    
    if (ws.lmxClosed === true) {
      return;
    }
    
    ws.lmxClosed = true;
   
    
    this.connectedClients.delete(ws);
    
    const v = this.wsToKeys.get(ws);
    this.wsToKeys.delete(ws);
    
    const uuids = Object.keys(this.wsToUUIDs.get(ws) || {});  //  here xxx
    this.wsToUUIDs.delete(ws);
    
    this.locks.forEach((v, k) => {
      
      const notify = v.notify;
      
      for (const uuid of Object.keys(uuids)){
        notify.remove(uuid);   // if a connection drops, we remove the request uuids for any existing requests
      }
      
      if (v.isViaShell !== true) {
        this.unlock({force: true, key: k, from: 'client socket closed/ended/errored'}, ws);
      }
      else if (!v.keepLocksAfterDeath) {
        this.unlock({force: true, key: k, from: 'client socket closed/ended/errored'}, ws);
      }
      
    });
    
  }

so it looks like we need to cleanup the uuids from that map after a request is done

from live-mutex.

ok I made a patch, try this new version:

• [email protected]

and let me know if the memory leak problem is gone, thanks

there is probably one more memory leak in there somewhere - there's only 3000 lines of code but it gets complex - if you see the memory profile grow, let me know.

from live-mutex.

Thanks for patching!

I didn't mean that "wsToUUIDs" map isn't used at all, but that it has no real purpose.
You never do something like this in the code:

const value = this.wsToUUIDs.get(ws)[uuid];
// Do something with value

All you do is pushing objects into a map and then cleaning it up without further elaborations, which will be something like:

{
'0a0e6fa3-b384-4295-9c7e-6163a8019e95': true,
'0a2e9211-4ba2-4050-989e-e4482edf7c80': true,
'0a4b4ca3-c392-4d2e-96ed-a0edcc8c080f': true,
...
}

What does "true" means?
Could those values be anything than true? Does it really matter that a specific UUID has a value "true"? Does other parts of code really care?
It doesn't seem so.
Besides this, it is ok to maintain that Map as long as it is cleaned correcly.

Your patch mitigated the Memory Leak by a large factor, but some elements still remains on "wsToUUIDs" map without being collected.

I wrote a simple test application that created a certain amount of lock and unlocks and remains alive so that memory could be inspected after forcing Garbage Collector to run.
I've seen that a specific element of contained in the following map (just one element, not all of them) retains some data:

the data is stored in this attribute:

with an internal structure similar to "wsToUUIDs" map.
I noted also "wsToUUIDs" map still had some elements and the size of the two maps were identical.
I checked and they both contain the same data (I mean, a copy, not a reference).

A further inspection showed that the leaking "UUIDs objects" were those where unlock was called using parameter "force" instead of a specific ID.
I relied on "force" option for convenience, but I suppose it is a better idea to save and pass correct ID.
Anyway, that map's values are not garbage collected correctly in such situation.
You should implement some code to clean it up.

from live-mutex.

thanks, I am going to implement some more cleanup as you mentioned. The wsToUuids maps are being read from, as I said above, when a client disconnects, any pending lock requests are removed from the request queue. That code is above.

Instead of:

{
`[key]: true`
}

I will probably just use a new Set(), using an object with key/value is more flexible though if I need to refactor which is why I use it sometimes instead of Set etc.

from live-mutex.

@m-lawliet btw, what technique/tool were you using to capture the memory for certain data structures at runtime? is it a chrome-based tool?

from live-mutex.

thanks for helping out with this

from live-mutex.