Memory leak when clients connected and disconnected

[richievos]

I have a simple MqttBroker setup, listening on a socket. I have been doing some testing that requires repeated connection opening/closing and I'm getting weird behavior. I added some more debugging and found there appears to be a memory leak related to TinyMqtt.

To reproduce this I am doing the following:

$ while true
do
mosquitto_pub -h <my_esp> -p 1883 -t foobar -m '{}'

done

I then have my esp periodically dumping memory stats. Effectively:

// every second
Serial.print("free_heap=");
Serial.println(xPortGetFreeHeapSize());

With that, I'm seeing every time I connect I have less memory available.

EDIT: I had some thoughts below on what was causing this, but I now found I was incorrect. I'm still seeing the memory leak, but I'm not clear what's causing it.

My guess is it's not as simple as the clients list not getting purged (I think I understand how that's happening now), but that the MqttClient itself isn't getting cleaned up.

[richievos]

Erronenous analysis below, for longevity (I removed this from the description because I found it's incorrect)

Looking at the TinyMqtt code, I'm not familiar with Arduino/ESP32 networking, but I'm confused that I don't see any code that ever appears to remove a client.

Walking through the add code, for the normal mode (without TINY_MQTT_ASYNC):

void MqttBroker::loop()
{
#ifndef TINY_MQTT_ASYNC
  WiFiClient client = server->accept();

  if (client)
  {
    onClient(this, &client);
  }
#endif
...
}

void MqttBroker::addClient(MqttClient* client)
{
  debug("MqttBroker::addClient");
  clients.push_back(client);
}

In the disconnect case, this cleanup code never seems to actually remove the client from the clients list:

void MqttBroker::loop()
{
....
  for(size_t i=0; i<clients.size(); i++)
  {
    MqttClient* client = clients[i];
    if (client->connected())
    {
      client->loop();
    }
    else
    {
      debug("Client " << client->id().c_str() << "  Disconnected, local_broker=" << (dbg_ptr)client->local_broker);
      // Note: deleting a client not added by the broker itself will probably crash later.
      delete client;
      break;
    }
  }

[dg6nee]

yes, this is what I found too, that seems to be the root cause and solved it in my fork (long time ago, PR23 AFAIR). The "release" of the client is done somewhere else, but sometimes the pointers got lost. My version runs on two devices for weeks, the official version stops working after less than one day (reproducible).

I have seen that someone has tried to fix this through use of auto pointers, but that seems not to work reliably either.

[richievos]

Can you link me the branch/tag/... you're using?

[richievos]

Debugging more, I think at least part of the issue is this:

    case MqttMessage::Type::Disconnect:
      // TODO should discard any will msg
      if (not mqtt_connected()) break;
      resetFlag(CltFlagConnected);
      Serial.println("disconnect");
      close(false);
...

that final close(false) is going to remove the client from the list when it's disconnected. When that happens, I don't think anything has a handle to it anymore, so it can't be cleaned up.

Diff incoming.

Diff is #74

[dg6nee]

Can you link me the branch/tag/... you're using?

https://github.com/dg6nee/TinyMqtt/tree/pr23_2

[richievos]

Ok, with some more minor tweaks to my branch, at least for me it's completely stable at the moment. I have two remote clients (mosquitto_pub on my laptop) connecting to my esp32 running a mqtt broker + a local client in the same process, and I'm seeing memory usage be stable. Also not seeing any weird crashes or free errors like I had triggered a couple times.

Also I should note, for me to be able to work cleanly I needed #72 and #74. My main branch has both of those on it, and otherwise are on top of hsaturn/main.

@dg6nee give my branch (richievos/main) a try at some point.

[richievos]

Follow-up 24hrs later. I haven't been fully stress testing the device like I was yesterday, but I have not seen any anomalous memory behavior since introducing #74 (on top of #72). I'm sure there's other memory leaks in here, but at least for the use-case of lots of short-lived tcp clients, I believe my PR fixes the issue.

[hsaturn]

Hello

Thanks for the report. Just to say that I'll have a look this evening (in 10 hours).

[hsaturn]

... working on it right now !

[hsaturn]

With examples/tinyqtt-tests.ino + MobaXTerm, I clearly saw that each client trying to connect and then disconnect was making a memory leak. The number of clients increases each time I click on Mqtt-Explorer 'connect'. This is easy to reproduce with as the -5000ms bug preventing clients from connecting correctly.

TinyMqtt tests shows periodically the number of clients connected as well as free memory. The free mem goes down when connecting and up when disconnecting. For me all is ok.

With the main branch (since https://github.com/hsaturn/TinyMqtt/commit/6b9d764c2309c640c8d0fcca0c52d25193e358a5) I cannot see any memory leak.

[richievos]

I haven't tested this yet, but can you clarify if you fixed a different leak in that commit you referenced or of you're saying you fixed the leak I reported and fixed in #74 ?

I'm assuming you're fixing a different bug based on the code changes and your earlier statements. However your recent comment leads me to believe you're saying it fixes both.

[richievos]

I should note, I don't think that commit would fix the bug I was encountering and I fixed in #74, but I'm happy to be proven wrong.

[hsaturn]

Hello Richie

I do not consider yet this issue (#73) as closed. I've fixed a memory leak when multiple clients are connecting and disconnecting. This could be, or not this issue. I have to conduct more tests. Last week, mosquitto couldn't even connect to TinyMqtt (-5000ms bug). So I could not test your scenario.

[szkezolt]

Hi Everyone!

I'm very intrested about this library. I used it long mounths ago with enc28j60 /tobozo with cable connection between my devices. Sometimes when i pull out a clients cable, the broker is freezing and stop working. I knoew this is not a normally using of tinymqtt beacuse i paired this with enc library. I begin to listen the free heap and when the client disconnect the memory is begin decraese until it totaly stop working. Still anyone similar problem?