Memory leak ?

[dbg981]

Hi,

In i'm writing a jxm client app in node i noticed a memory leak issue (the process memory increased to 500MB+ in a couple of hours) Using heapdump and Chrome Dev Tools i traced it to jx_obj.RequestList.s in jxm_client.js which kept growing. I also verfied this by debugging the app.

Looking a bit inside the code i didn't find any obvious way that array was being trimmed.

Here's a quick fix i came up with to make my app work:

backend/jxm_client.js:

Line 24 - made s an object instead of an array (there are no obvious array methods used on it)

jx_obj.RequestList = { s: {}, l: {}, sc: 0, sn: 0, ready: true, force_false: false };

Line 440 - appended delete line to delete used requests

    if (jx_obj.SocketOpen) {
        jx_obj.Socket.send(jx_obj.RequestList.s[call_id].message);
    } else {
        jx_obj.SFirst.push(jx_obj.RequestList.s[call_id].message);
    }
    delete jx_obj.RequestList.s[call_id];
    return true;
};

Line 503 - appended delete line to delete used requests

    if (jx_obj.RequestList.s[call_id] != null) {
        txt.push('ms=' + jx_obj.RequestList.s[call_id].message);
        delete jx_obj.RequestList.s[call_id];
    }

Maybe only deleting the message after jx_obj._Send(_pool); at line 237 would have been enough.

My app connects to server, subscribes to multiple groups (by calling server custom method) and starts pushing messages to those channels.

Is there a better way to avoid this issue?

[ktrzeciaknubisa]

Well, that may be a good start, but let's be careful with too much deleting, otherwise this may prevent retrying to send a message if connection gets interrupted (leading to loss of messages). You may do a test for yourself when connection breaks shortly and count messages if they all arrive.

Deleting should take place once we're sure, that message was truly sent.

[obastemur]

@ktrzeciaknubisa this looks like a critical issue. Let's prioritize it ?

[ktrzeciaknubisa]

I assume that also browser client needs to be checked. This particular issue was rised for node client, which originates from browser version.

[dbg981]

I looked a bit more and it seems that: Items are pushed in jx_obj.RequestList.s at line 211 The next piece (at line 234) sets the pointer of what will be read

            if (jx_obj.RequestList.sn > jx_obj.RequestList.sc) {
                var _pool = jx_obj.RequestList.sc;
                if (jx_obj.RequestList.s[_pool] != null) {
                    jx_obj._Send(_pool);
                }
                jx_obj.RequestList.sc++;
            }

and it looks like the pointer jx_obj.RequestList.sc-->_pool is only going up. I could find no other occurrences of it.

The array will be read only by calls from jx_obj._Send(_pool); above:

• line 500 in jx_obj._Send(call_id) called from above:

    if (call_id != -1) {
        try {
            if (jx_obj.RequestList.s[call_id] != null) {
                txt.push('ms=' + jx_obj.RequestList.s[call_id].message);
            }
        } catch (e) {
            return;
        }
    }

• line 435 in jx_obj.GoSockets(call_id) via jx_obj._Send(call_id,..)

    if (jx_obj.SocketOpen) {
        jx_obj.Socket.send(jx_obj.RequestList.s[call_id].message);
    } else {
        jx_obj.SFirst.push(jx_obj.RequestList.s[call_id].message);
    }

here i guess the jx_obj.SFirst caches the messages in case of disconnection.

In both cases call_id is passed from _pool above.

Anyway, it looks like after .sc is incremented, no other code will access what's behind it. If access should happen on the deleted items, it would end in a undefined exception since i only see checks for null.

These segments look identical in browser code (nothing seems to free that array), but i could not test since my frontend part only receives messages for now.

I'll try to test what happens in case of disconnection.

[ktrzeciaknubisa]

Thanks @dbg981 for a deep analysis. I've also looked into it. Looks like your first proposal may be sufficient after all.

Maybe only deleting the message after jx_obj._Send(_pool); at line 237 would have been enough.

This seems right too.

I'm also going to run my own tests.

[ktrzeciaknubisa]

@dbg981 After some tests I think the two of your proposals are sufficient and good to be implemented:

jx_obj.RequestList = { s: {}, l: {}, sc: 0, sn: 0, ready: true, force_false: false };

if (jxcore.RequestList.s[_pool] != null) {
    jxcore._Send(_pool);
    delete jxcore.RequestList.s[_pool];
}

Would you like to make a PR with that and become a contributor? :)

[dbg981]

Made PR. Sorry for delay... Lots of work and had to figure out how github works :P

[ktrzeciaknubisa]

No problem, thanks!

[ktrzeciaknubisa]

@obastemur I think we can publish jxm master to npm. Version number (4.0.0) updated in package.json. Apart from above fix there were already some fixes added before.