Memory leak in publish operation #1345
Comments
|
and what about on current code? you filed the ticket here, why didn't you look at the code here instead of some years old independent fork? |
First of all, I just wrote the trace back for hint. I`m not sure but I think there is a back trace for scenario 2 like this: https://github.com/eclipse/mosquitto/blob/master/lib/packet_mosq.c#L219 |
|
Thanks for the report. Do you have some example code that shows this behaviour? I've just tried to implement what I think you mean, but haven't been able to reproduce it. I've tried on version 1.6.3 and 1.4.8. |
First, sorry for the late reply. I have written a poc and compile that. The problem still exists. mqtt.h #ifndef MQTT_H
#define MQTT_H
#include <string>
#include <cstdio>
#include <cstring>
#include <iostream>
#include <mosquittopp.h>
using namespace std;
#define DEAFULT_KEEP_ALIVE 60
class mqtt : public mosqpp::mosquittopp
{
public:
mqtt(const char* id);
int init_mqtt(string host, int port);
void on_connect(int rc);
void on_disconnet(int rc);
};
#endif //MQTT_Hmqtt.cpp #include "mqtt.h"
using namespace std;
mqtt::mqtt(const char *id) : mosquittopp(id)
{
}
int mqtt::init_mqtt(string host, int port)
{
string log;
int ret;
mosqpp::lib_init();
ret = connect_async(host.c_str(), port, 60);
if(ret != MOSQ_ERR_SUCCESS)
{
if(ret == MOSQ_ERR_ERRNO)
{
log = "(" + host + ":" + to_string(port) + "): connect_async failed! (" + to_string(errno) + ": " + strerror(errno) + ")";
cout << log << endl;
return __LINE__;
}
else
{
log = "(" + host + ":" + to_string(port) + "): connect_async failed! (" + to_string(ret) + ": " + string(mosquitto_strerror(ret)) + ")";
cout << log << endl;
return __LINE__;
}
}
ret = loop_start();
if(ret != MOSQ_ERR_SUCCESS)
{
if(ret == MOSQ_ERR_ERRNO)
{
log = "(" + host + ":" + to_string(port) + "): loop_start failed! (" + to_string(errno) + ": " + strerror(errno) + ")";
cout << log << endl;
return __LINE__;
}
else
{
log = "(" + host + ":" + to_string(port) + "): loop_start failed! (" + to_string(ret) + ": " + string(mosquitto_strerror(ret)) + ")";
cout << log << endl;
return __LINE__;
}
}
}
void mqtt::on_connect(int rc)
{
if(rc == MOSQ_ERR_SUCCESS)
cout << "Connected successfully to the broker" << endl;
else
cout << "Error in connection to the broker" << endl;
}
void on_disconnet(int rc)
{
if(rc == MOSQ_ERR_SUCCESS)
cout << "Disconnected successfully from the broker" << endl;
else
cout << "Error in disconnection from the broker" << endl;
}main.cpp #include <cstring>
#include <cstdio>
#include <string>
#include <iostream>
#include <cstdlib>
#include <unistd.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <arpa/inet.h>
#include <netinet/in.h>
#include <memory>
#include <fstream>
#include <sys/stat.h>
#include "mqtt.h"
using namespace std;
int main(int argc, char* argv[])
{
int ret = 0;
string log;
string id = "my_id";
int port = 6101;
string host = "localhost";
int mid;
string file_path = "250MB.bin";
string topic = "my_topic";
int qos = 0;
bool retain = false;
mqtt my_mqtt(id.c_str());
my_mqtt.init_mqtt(host, port);
struct stat path_stat;
stat(file_path.c_str(), &path_stat);
long payloadlen = path_stat.st_size;
unique_ptr<char []> buffer(new char[payloadlen]);
FILE *fp = fopen(file_path.c_str(), "rb");
if(fp == NULL)
{
log = "fopen failed in file '" + file_path + "': " + strerror(errno);
cout << log << endl;
}
long nread = 0;
long total_nread = 0;
while(total_nread < payloadlen)
{
nread = fread(buffer.get() + total_nread, sizeof(char), payloadlen - total_nread, fp);
total_nread += nread;
}
fclose(fp);
cout << "[After 3 publications] First please check the RAM usage (free -m) and then press any key to continue..." << endl;
getchar();
// publish 3 times
int published_ret = my_mqtt.publish(&mid, topic.c_str(), payloadlen, buffer.get(), qos, retain);
if(published_ret == MOSQ_ERR_SUCCESS)
{
log = "File '" + file_path + "'published successfully";
cout << log << endl;
}
else if(published_ret == MOSQ_ERR_SUCCESS)
{
log = "File '" + file_path + "' is not published (" + to_string(published_ret) + ": " + strerror(errno) + ")";
cout << log << endl;
}
else
{
log = "File '" + file_path + "' is not published (" + to_string(published_ret) + ": " + string(mosquitto_strerror(published_ret)) + ")";
cout << log << endl;
}
published_ret = my_mqtt.publish(&mid, topic.c_str(), payloadlen, buffer.get(), qos, retain);
if(published_ret == MOSQ_ERR_SUCCESS)
{
log = "File '" + file_path + "'published successfully";
cout << log << endl;
}
else if(published_ret == MOSQ_ERR_SUCCESS)
{
log = "File '" + file_path + "' is not published (" + to_string(published_ret) + ": " + strerror(errno) + ")";
cout << log << endl;
}
else
{
log = "File '" + file_path + "' is not published (" + to_string(published_ret) + ": " + string(mosquitto_strerror(published_ret)) + ")";
cout << log << endl;
}
published_ret = my_mqtt.publish(&mid, topic.c_str(), payloadlen, buffer.get(), qos, retain);
if(published_ret == MOSQ_ERR_SUCCESS)
{
log = "File '" + file_path + "'published successfully";
cout << log << endl;
}
else if(published_ret == MOSQ_ERR_SUCCESS)
{
log = "File '" + file_path + "' is not published (" + to_string(published_ret) + ": " + strerror(errno) + ")";
cout << log << endl;
}
else
{
log = "File '" + file_path + "' is not published (" + to_string(published_ret) + ": " + string(mosquitto_strerror(published_ret)) + ")";
cout << log << endl;
}
cout << "[After 3 publications] And now, please check again the RAM usage (free -m) and then press any key to exit..." << endl;
getchar();
return 0;
} The poc tries to connect to (localhost:6101) which no broker is listening to. Before executing the program the RAM usage is as show below: Before publishing the RAM usage is as show below: And after publishing the RAM usage is as show below: Used RAM: 1252 - 249 =~ 1000MB --> 250MB (The file in RAM) + 250MB (MeMLeak from publication number 1) + 250MB (MeMLeak from publication number 2) + 250MB (MeMLeak from publication number 3) |
|
run it under valgrind if you have such a nice test case, it's much more useful at finding what's going on than free, which looks at the entire system |
|
I haven't had chance to look at this yet because of the sheer number of compiler errors there are. It's all very well apologising for the typos and mistakes in your test case, but it suggests that you haven't actually tested the test case and are hoping it shows what you want, and besides that I find it very impolite that you expect anybody who would like to help you to fix all of your errors first. My guess is that what you are seeing is similar to what this shows: https://github.com/ralight/heap-allocation/blob/master/mem.c I will fix the errors later and see what I can find. |
As I said, I compiled my test case and sent you the result as shown above. I`ve got shocked with your non-professional behaviour and unfriendly words. Agian, as I said, becuase of some problems, I have not direct access to my developing system and I have to rewrite the code manually in my internet system and then send to you. Anyway, I`ve editted codes of the test case and think there will be no execuses! |
|
I guess we're even then, we're both shocked by each others lack of consideration. It is quite wearing that so many people contacting me expect me to do all of the work for them, and I apologise that you have been the recipient of my frustration. This isn't a memory leak, it is just queueing the packets when it shouldn't be. If you connect again or free the client, the memory will be freed. I agree that it shouldn't be held in limbo like this though. If you use connect() instead of connect_async(), the memory is not held at all. I have a patch that will make connect_async() work just the same as connect() in this regard. |
`mosquitto_connect_async()` is now consistent with `mosquitto_connect()` when connecting to a non-existent server. Closes #1345. Thanks to Mohammad Reza.
|
People contant you because you did a great work: Mosquitto lib in C/C++! Anyway, thank you for consideration and the trying to make this lib better and better. |
`mosquitto_connect_async()` is now consistent with `mosquitto_connect()` when connecting to a non-existent server. Closes eclipse#1345. Thanks to Mohammad Reza.
`mosquitto_connect_async()` is now consistent with `mosquitto_connect()` when connecting to a non-existent server. Closes #1345. Thanks to Mohammad Reza.
Consider these two scenarios:
Scenario 1:
In this scenario there is no memory leak. But pay attention to this scenario:
Scenario 2:
This bad error handling leads to a memory leak. According to the QoS of the message, size of memory leak will be differ. If QoS is 0, the memory will be consumed as much as the payloadlen. If the QoS is 1, the size will be multiplied by two.
For example:
QoS=0, payloadlen=250MB --> Consumed memory in memory leak: 250MB
QoS=1, payloadlen=250MB --> Consumed memory in memory leak: 500MB
According to the code available on https://github.com/iosphere/mosquitto, I did these back trace:
Scenario 1:
https://github.com/iosphere/mosquitto/blob/master/lib/send_mosq.c#L103
if(mosq->sock == INVALID_SOCKET) return MOSQ_ERR_NO_CONN;_mosquitto_send_publish (send_mosq.c#L103)
mosquitto_publish (mosquitto.c.)
publish (mosquittopp.cpp)
Scenario 2:
https://github.com/iosphere/mosquitto/blob/master/lib/net_mosq.c#L799
return MOSQ_ERR_ERRNO;_mosquitto_packet_write (net_mosq.c#L799)
_mosquitto_packet_queue (net_mosq.c)
_mosquitto_send_real_publish (send_mosq.c)
_mosquitto_send_publish (send_mosq.c)
mosquitto_publish (mosquitto.c.)
publish (mosquittopp.cpp)
Note
Maybe you say that is the publisher fault. He must check the connection status and then publish his message.
Yes, but the library is handling this exception and returning 'MOSQ_ERR_ERRNO' error message. So, it must complete the task and clean up the memory. otherwise, it seems a segmentation fault is better than a bad exception handling.
The text was updated successfully, but these errors were encountered: