Continuous SUBSCRIBE cause memory consumption rise

[mocheng]

This happens in production. One bug in our client side exposed this problem.

When a client failed to subscribe a topic, it just keeps reconnecting and continue subscribing. However, there is a bug in client side application. Every time it reconnects, it send 2 more SUBSCRIBE packets. That is, Mosca server got a lot of CONNECT and then a bunch of SUBSCRIBE commands.

This causes memory consumption rise as below image:

We reproduce this issue and got some heapdump. There are a lot of WriteReq objects created during the storming of SUBSCRIBE. Something wrong in stream?

Even though this is caused by a client side bug, it could be a weakness point for malicious attack.

[mcollina]

Thanks for reporting this! It's really the kind of issues I would like to get solved inside Mosca.

I think this happens because the connections are buffering data to be sent, and those are never deleted correctly.
Can you make it run some more in production? I would like to know if this stops after a while, or just continues growing forever, e.g. there is some timeout mechanism that cleans up or stabilize the mess.

Can you replicate it using just MQTT.js, and upload the gist? So I can try solving it here? Thanks.
Maybe you can use the mqtt.createConnection() method, to create a misbehaving client.

I think it may be related to #170, i.e. we are not disconnecting clients anymore for unauthorized subscribes.

[mocheng]

I think it may be related to #170, i.e. we are not disconnecting clients anymore for unauthorized subscribes.

In production, we are still on 0.22.0 which doesn't include #170 .

I'm still digging the root cause. Will post findings later.

[mcollina]

One more question: which version of node.js are you running?

[mocheng]

node version is v0.10.28

[mocheng]

The code that plays cracker role is like this https://gist.github.com/mocheng/d5d7aa532c12c8ac8b24
Basically, it keeps sending incremental count of SUBSCRIBE command.

[mocheng]

This might be due to one problem in our application code. For easy debugging, we have below code to hook mqtt.MqttConnection.prototype._parsePayload. The parsed packet is logged by log4js.

var mqtt_lib = require(path_to_mqtt);

function log_receiving_packets() {
    var parsePayload = mqtt_lib.MqttConnection.prototype._parsePayload;
    mqtt_lib.MqttConnection.prototype._parsePayload = function(parse) {
        logger.mosca.info('@@@receive message: ' + this.packet.cmd);
        var result = parsePayload.call(this, parse),

        logger.mosca.info(result);

        return result;
    };
}

After removing lines that have logger.mosca.info, the gist https://gist.github.com/mocheng/d5d7aa532c12c8ac8b24 storming with SUBSCRIBE doesn't cause Mosca server memory grow.

I'm not clear about the reason, but it seems there is some closure memory leak here.

[mcollina]

This really depends on where you put that code. It does not look like problematic, but it really depends on where you create that closure. If it creates a reference loop between objects, you might screw the garbage collector.

Anyway, monkey patching libraries is usually a very bad idea. You can get the same feature by hooking mqttConnection.on(<command name>). Moreover, that method is dynamically built and highly-optimized, as it's a really hot point int MQTT.js, so better not touch it.

Let me know if we can consider this solved.
(I'm happy to help anyway).

[mocheng]

After removing the code leads to leak, I ran another script storming Mosca with PUBLISH as https://gist.github.com/mocheng/d5d7aa532c12c8ac8b24#file-storm-with-publish . Then, the Mosca server memory consumption rises rapidly.

I guess this is expected. Mosca server is not supposed to handle limitless PUBLISH request, right?

[mcollina]

It should rise rapidly, as data arrives, but it should stabilize and eventually reduce it when it's relieved. Or the memory consumption leads to
The default node thing is 'allocate as there is need, then clean it up'. Conclusion: it will eat up all your memory under stress.

[mcollina]

Plus, the current version of Mosca cannot handle backpressure over MQTT, but it will.

[mocheng]

It stabilizes and dropped to OK level after pressure is relieved, though it takes about 15 minutes.

Work as expected.

[mcollina]

Anyway, it's a backpressure issue. Data is arriving too fast for Mosca, and it should slow it down and eventually disconnect the offending client. Unfortunately the 'bug' is in MQTT.js, so I'll try solving that first, and then we'll see.