knolleary / pubsubclient

A client library for the Arduino Ethernet Shield that provides support for MQTT.
http://pubsubclient.knolleary.net/
MIT License
3.82k stars 1.47k forks source link

Memory leak when connect() #673

Open edodad opened 4 years ago

edodad commented 4 years ago

Hi following the sample, I noticed that the library uses 1KB aprox of heap every time I call connect()

so for example:

void loop() { ... if ( !pubSubClient.connected()) { mqttConnect(); } ... }

and

void mqttConnect() { ... if (pubSubClient.connect(clientid, user, password)) { ... } else { ... } ... }

It means that every time my router lost tinternet connection, pubSubClient.connected() is FALSE, mqttConnect() is called and THEN it butns 1KB of heap every time, without releasing the OLD memoty

How can I solve the issue?

Thannks

edodad commented 4 years ago

Arduino Core 2.5.2 pubsubclient 2.7.0

Speedy2k commented 4 years ago

I seems to have a similar issue but on the long run, it seems to be after around 24 hours of running, i don't know why, it try to reconnect and it jam in the connect function and never get out of there, i don't exactly know what is happening since i don't get any error, the watchdog on my esp32 doesn't kick. Everything just stop!?

knolleary commented 4 years ago

The PubSubClient code is very stable and has not changed for some time. No-one else has seen this issue, so there must be something about your sketch, or the particular network client you are using.

Looking at your code (which you've sent me via email) to see where you are allocating additional memory each time it connects I can see you are creating a new instance of the WiFiClientSecure and PubSubClient objects every time you try to reconnect. Similarly you generate a new client ID every time. You could try statically assigning them at the start of the script rather than recreating them every time - this is the approach all of the example sketches take.

TD-er commented 4 years ago

Well, I have some reports of users with poor WiFi connections, stating when MQTT (thus this library) is used, they run out of memory and thus crash. (example: https://github.com/letscontrolit/ESPEasy/issues/2684 )

About recreating the WiFiclient. That's also what I'm doing and was suggested here: https://github.com/esp8266/Arduino/issues/4497#issuecomment-373023864

So I will also have a look at the progress of this issue.

narenayak commented 4 years ago

Newbie so plz correct me if I am wrong to comment on this issue it may be a different issue but is related to esp32 and connect(). uncommented one new code where i had to use a flag instead of mqttClient.connect() in an if/else.My problem was solved by doing this Commented part indicates what I was doing earlier . The problem I was facing on esp32 hardware was is as mention below : Wifi will be available on boot.Broker not available. At this point when it enters the if (mqttClient.connect(xyz) ) i disconnected the wifi and my esp32 used to hang here, never used to come out of it.Basically I had to use external reset or power recycle. i tried added a wifi available flag (network_connected_flag()) which never helped. Other details: SDK Version:v3.3-beta1-506-gebdcbe8c6 IDF Version:v3.3-beta1-506-gebdcbe8c6 pubsubclient: 2.8 Arduino 1.8.5

if ((mqtt_user != "" && mqtt_passwd != "" && mqttClient.connect((char*) hostName.c_str(), (char*)mqtt_user.c_str(), (char*)mqtt_passwd.c_str(), (char*)pubTopic.c_str(), 0, 0, (char*)dynamic_will_msg.c_str())) && network_connected_flag()) //added network_connected_flag()on 07/08/2020
  {
   Serial.println("MQTT Connected via username & password");
   portENTER_CRITICAL(&timerMux);
   cTime =0;
   portEXIT_CRITICAL(&timerMux);
   configured_flag=1;
   mqttClient.publish((char*)pubTopic.c_str(),(char*)dynamic_connected_msg.c_str());
   mqttClient.subscribe((char*)subTopic.c_str());
   return true;
  }
  else if((mqttClient.connect((char*) hostName.c_str(),(char*)pubTopic.c_str(), 0, 0, (char*)dynamic_will_msg.c_str())) && network_connected_flag())  //added network_connected_flag()on 07/08/2020
  {
   Serial.println("MQTT Connected via no username & password");
   portENTER_CRITICAL(&timerMux);
   cTime =0;
   portEXIT_CRITICAL(&timerMux);
   configured_flag=1;
   mqttClient.publish((char*)pubTopic.c_str(),(char*)dynamic_connected_msg.c_str());
   mqttClient.subscribe((char*)subTopic.c_str());
   return true;
  }

Solved it by using a flag instead of trying to connect it in the if loop , I am not sure if this method is correct.Please correct me if i am wrong . As of now it is working fine probable i will test it this week and post if there is some problem.

boolean connectMQTT() 
{  
   //Serial.println("Entering MQTT Connect");
   bool mqttclient_connected_flag;
   String device_id =hostName;
   //device_id.replace("-","");
   String dynamic_will_msg=deviceName+"--"+mqtt_will_msg;
#ifdef STANDARD_TIME
   //String dynamic_will_msg = "{\"state\": {\"reported\":{\"Client ID\":\""+device_id+"\",\"Device Name\":"+"\""+deviceName+"\""+",\"Status\":\"Disconnected\"}}}";
   String dynamic_connected_msg = "{\"state\": {\"reported\":{\"Client ID\":\""+device_id+"\",\"Device Name\":"+"\""+deviceName+"\""+",\"Status\":\"Connected\","+sG_Present_Timestamp_std+","+S_bootreason+","+S_resetreason[0]+","+S_resetreason[1]+",\"SWRebootCause\":"+sw_reset_reason+"}}}";
#elif defined  EPOCH_TIME
  //String dynamic_will_msg = "{\"state\": {\"reported\":{\"Client ID\":\""+device_id+"\",\"Device Name\":"+"\""+deviceName+"\""+",\"Status\":\"Disconnected\"}}}";
  String dynamic_connected_msg = "{\"state\": {\"reported\":{\"Client ID\":\""+device_id+"\",\"Device Name\":"+"\""+deviceName+"\""+",\"Status\":\"Connected\","+sG_Present_Timestamp_epoch+","+S_bootreason+","+S_resetreason[0]+","+S_resetreason[1]+",\"SWRebootCause\":"+sw_reset_reason+"}}}";
#endif
   if (mqttClient.connected() && network_connected_flag()) //added network_connected_flag()on 07/08/2020
   {
    mqttClient.loop();
    mqtt_retries =0;
    portENTER_CRITICAL(&timerMux);
    cTime =0;
    portEXIT_CRITICAL(&timerMux);
    configured_flag=1;
    return true;
   }
   else
   {
    mqtt_retries++;
     if(mqtt_retries > MAX_MQTT_RETRY)
       {
        Serial.print("Going to reboot as mqtt failed for more than 15 times....."); 
        timerAlarmDisable(timer_one);
        Serial.print("Reason code written ");
        delay(10);
        esp_restart(); 
       }
    }
   Serial.print("Connecting to MQTT Broker with Client ID:");
   Serial.println(hostName);
   Serial.print("Connecting using user Name::Password::Will Topic::Will msg with Client ID:");
   Serial.println("--"+mqtt_user+"::"+mqtt_passwd+"::"+pubTopic+"::"+dynamic_will_msg);

   if (mqtt_user != "" && mqtt_passwd != "")
   {
    Serial.println("MQTT Connecting via username & password");
    mqttclient_connected_flag = mqttClient.connect((char*) hostName.c_str(), (char*)mqtt_user.c_str(), (char*)mqtt_passwd.c_str(), (char*)pubTopic.c_str(), 0, 0, (char*)dynamic_will_msg.c_str()) & network_connected_flag();
    Serial.println(mqttclient_connected_flag);
   }
   else
   {
    Serial.println("MQTT Connecting via no username & password");
    mqttclient_connected_flag = mqttClient.connect((char*) hostName.c_str(),(char*)pubTopic.c_str(), 0, 0, (char*)dynamic_will_msg.c_str()) & network_connected_flag();
    Serial.println(mqttclient_connected_flag);
   }

  if (mqttclient_connected_flag) //added network_connected_flag()on 07/08/2020
  {
   Serial.println("MQTT Connected");
   portENTER_CRITICAL(&timerMux);
   cTime =0;
   portEXIT_CRITICAL(&timerMux);
   configured_flag=1;
   mqttClient.publish((char*)pubTopic.c_str(),(char*)dynamic_connected_msg.c_str());
   mqttClient.subscribe((char*)subTopic.c_str());
   return true; 
  }
  else
  {
    Serial.print("MQTT not Connected, rc=");
    Serial.print(mqttClient.state());
    Serial.print(", retry count is ");
    Serial.print(mqtt_retries);
    Serial.println(", waiting 120 seconds to retry");
    sw_reset_reason="\"mrc="+String(mqttClient.state())+"\""; //added on 07/08/2020
    portENTER_CRITICAL(&timerMux);
    configured_flag=0;
    portEXIT_CRITICAL(&timerMux);
    disconnectMQTT();
    //delay(500);
    Debugln("DEBUG:Configuration Flag LOW");
    if(mqtt_retries> (MAX_MQTT_RETRY-1)) //added on 07/08/2020
    {
    Serial.print("Saving mqtt failed status to memory...."); //added on 07/08/2020
    saveConfig() ? Serial.println("sucessfully.") : Serial.println("not succesfully!");; //added on 07/08/2020
    }
    return false;
  }
  //configured_flag=0;
  //Serial.println("Configuration Flag LOW");
  //disconnectMQTT();
  //delay(500);
  //return false;
}
bool network_connected_flag()
{

//#ifdef  OnWiFi  
  if(WiFi.status() == WL_CONNECTED )
  {
    return true;
  }
  else
  {
    return false;
  } 
}
TD-er commented 4 years ago

Can you use 3 backticks at begin and end of a code block? You're now using only 1 backtick, which is meant for inline code.

narenayak commented 4 years ago

Can you use 3 backticks at begin and end of a code block? You're now using only 1 backtick, which is meant for inline code.

Thx corrected it