Memory leak due to removal of MqttClient objects from the clients list

[Kirlik]

The MqttClient::close() function removes the MqttClient object from the broker's client list. But the object is not destroyed and memory leaks over time. It looks like the parent.removeClient function should be moved from the close() function to the place where the MqttClient object is being deleted.

[rmey]

Does erase not only remove the MqttClient pointer from the vector? See also https://forum.openframeworks.cc/t/delete-objects-from-std-vector-elements-and-delete-the-objects-itself-also/19167/4

void MqttBroker::removeClient(MqttClient* remove)
{
  for(auto it=clients.begin(); it!=clients.end(); it++)
	{
		auto client=*it;
		if (client==remove)
		{
			// TODO if this broker is connected to an external broker
			// we have to unsubscribe remove's topics.
			// (but doing this, check that other clients are not subscribed...)
			// Unless -> we could receive useless messages
			//        -> we are using (memory) one IndexedString plus its string for nothing.
			debug("Remove " << clients.size());
			clients.erase(it);
			debug("Client removed " << clients.size());
			return;
		}
	}
	debug("Error cannot remove client");	// TODO should not occur
}

[hsaturn]

(quick answer I'm at work) removeClient must not delete remove ! remove is an observer of the broker. It does not belong to the broker. The borker ran for hours (days) without crashing so I guess there is no problem.

That said, I'll look if I can use unique_ptr instead of new / delete. Maybe shared by I'd prefer not to. There are a few couple of new/delete that maybe leads to some memory leaks elsewhere in the code.

[Kirlik]

But then who should delete them? these objects remain in memory and after a day the program with 5-10 clients crashes. I analyzed the memory and found out that it is a leak in this place that leads to a fall

[hsaturn]

As I said, I'll take a deeper look on this. Maybe you're right in that that a memory leak can occur, even if removeClient must be kpet unchanged.

[hsaturn]

Hello kirik

The problem is more serious than I thought.

Clients added in the first occurence of addClient must not be deleted Clients added in the 2nd occurence of addClient should be deleted.

Obviously this is a bad architecture.

Thanks for the report

[dg6nee]

Hi, any news on this topic?

I am suffering the same and try to understand what is going wrong. As far as I can see, the situation is quite tricky. I will try to analyze...

There are two ways how a client gets into the list of a broker.

1. Through a "constructor", e.g. with a static instance, like this: static MqttBroker g_mqtt_broker(1883); static MqttClient g_mqtt_client(&g_mqtt_broker); -> this "client" instance must never be deleted.

2. through MqttBroker::onClient(...) -> created with "new", must be cleaned up after use

Now, how does the live of a client end? The only way the client gets removed from the "clients" list of the broker is through MqttBroker::removeClient(...). This function is only called in MqttClient::close(...), which is called from several places. After calling "close", the pointer may no longer be in the list and therefore is lost. This is main topic we have to address.

On the other hand, the only place where a "delete" of the client is done is in the MqttBroker::loop() function.

BTW: this loop code iterates in forward direction over the list of clients. This is very dangerous, as the destructor of the MqttClient implicitly modifies the list, which is dangerous. In general it is much safer to iterate in reverse direction.

Additionally, all clients that have been set out of business through "close" are no longer in the list and therefore are lost forever.

As a conclusion, I think we have to address two things:

a) In removeClient() we should not really remove the client from the list, but instead mark it somehow as "dead - must be cleaned up". Then the loop function will act as some kind of garbage collector and do the final erase + delete.

b) This must not happen with a client that has entered through some constructor.

Probably there are several ways to solve this. One possible solution would be introducing two flags:

• one flag for "has been created with 'new' - must be cleaned up after use"
• one flag for "is dead - must be cleaned up"

What do you think, did I miss something?

[dg6nee]

should be fixed by pull request Pr23_2

With the original code my target crashed after some few minutes when opening connections with "mosquitto_sub -C 1 -R ...".

Now it runs for days without problems, with more or less constant amount of free memory.

[hsaturn]

I think this issue is fixed with the main branch. I'll release soon this branch after some tests.