Fix memory leak in MqttClient #1742
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
slaff
reviewed
Jul 4, 2019
|
This patch stops the reconnect attempts entirely for me. The first tries are there but are still decreasing memory (which is expected as far as I understand the change). Then the queue check kicks in no more connection attempts happen. NOTE: Tested on 3.8.0! |
slaff
reviewed
Jul 5, 2019
Sming/Core/Network/MqttClient.cpp
Outdated
| message->common.type = MQTT_TYPE_SUBSCRIBE; | ||
| message->subscribe.topics = (mqtt_topicpair_t*)malloc(sizeof(mqtt_topicpair_t)); | ||
| memset(message->subscribe.topics, 0, sizeof(mqtt_topicpair_t)); | ||
| mqtt_message_t message; |
There was a problem hiding this comment.
You are creating here a local variable and then you are passing the address of that local variable to be used later from the requestQueue. I am not sure this is a good idea. By the time the message is dequeued the memory address will most certainly point to something unexpected.
There was a problem hiding this comment.
I've taken another go at this, much happier with it.
There was a problem hiding this comment.
I've taken another go at this, much happier with it.
Now it looks much better :)
There was a problem hiding this comment.
On the Host it works like charm.
55018194 MQTT replacing connect message
MQTT Broker Disconnected!!
Connecting to mqtt://attachix.zom:1883/
57112580 MQTT replacing connect message
MQTT Broker Disconnected!!
Connecting to mqtt://attachix.zom:1883/
59217835 MQTT replacing connect message
MQTT Broker Disconnected!!
Connecting to mqtt://attachix.zom:1883/
61317774 MQTT replacing connect message
MQTT Broker Disconnected!!
...
and the final summary
==5606== LEAK SUMMARY:
==5606== definitely lost: 0 bytes in 0 blocks
==5606== indirectly lost: 0 bytes in 0 blocks
==5606== possibly lost: 0 bytes in 0 blocks
* Handle connect message outside of queue, always give it priority * In `copyString`, free destination buffer first to avoid memory leaks * Use helper functions to create, delete and clear messages * Use `MQTT_MALLOC` instead of `malloc` for messages and other items
|
@mikee47 Can you try again with the latest code from this PR? |
|
@verybadsoldier Can you try again with the latest code from this PR? |
|
Sorry but still leaking for me: |
|
Wait a bit, the queue holds 10 messages so memory will stop incrementing once that's full. |
|
Ah, thanks, right. Stable for a while then one drop later. But that might have had other reasons. |
The first is expected and the second is something that I have noticed multiple times. The updated PR addresses the memory leak issue and I plan to merge it later today or tomorrow if there are no objections. |
slaff
pushed a commit
to slaff/Sming
that referenced
this pull request
Jul 7, 2019
* Handle connect message outside of queue, always give it priority * In `copyString`, free destination buffer first to avoid memory leaks * Use helper functions to create, delete and clear messages * Use `MQTT_MALLOC` instead of `malloc` for messages and other items * Remove COPY_STRING macro and handle allocation failures
slaff
pushed a commit
to slaff/Sming
that referenced
this pull request
Jul 7, 2019
* Handle connect message outside of queue, always give it priority * In `copyString`, free destination buffer first to avoid memory leaks * Use helper functions to create, delete and clear messages * Use `MQTT_MALLOC` instead of `malloc` for messages and other items * Remove COPY_STRING macro and handle allocation failures
slaff
added a commit
that referenced
this pull request
Jul 7, 2019
* Handle connect message outside of queue, always give it priority * In `copyString`, free destination buffer first to avoid memory leaks * Use helper functions to create, delete and clear messages * Use `MQTT_MALLOC` instead of `malloc` for messages and other items * Remove COPY_STRING macro and handle allocation failures
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If request queue is full due to non-responsive server then allocated message will never be freed.
A new
MqttRequestQueueclass handles all message queueing. Only one connect message may be queued, and it always takes priority over others.Should fix #1740