Skip to content
This repository has been archived by the owner on Apr 6, 2019. It is now read-only.

Fix memory corruption in case of pending callbacks on destruction of client object #200

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

skeetor
Copy link

@skeetor skeetor commented Sep 6, 2018

When the client object is distroyed but the the callbacks are not yet completed, this causes a memory corruption as the callback counter variable is in nowhereland. So we have to wait until the callbacks have finished.

// [email protected]
// If there are pending callbacks we have to wait until they are completed
// otherwise this will cause a memory corruption if they return after this point.
wait_for_completion();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if no callbacks are pending, then notify_all() will not be called from connection_receive_handler and m_sync_condvar.wait will hang. So I don't think this works.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take that back. However, there is another problem: If there are uncommitted commands, then wait for completion will not return.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skeetor , have a look at this please https://github.com/steple/cpp_redis/commit/b69991470c4ea35b29279b79b8b836c0321ad315

This will pass all tests. I'll try to add a test case that tests for the original bug of the pending callback.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So does this mean that I can withdraw my PR and you will submit yours?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will, but my PR is missing a test case for the bug that you had found. I'll need to add that before I can submit it. If you have an idea on how to test it, please let me know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unable to reproduce what I think is the bug you described. Can you provide an example or a more detailed description?

@@ -288,7 +299,7 @@ client::resend_failed_commands(void) {
//! dequeue commands and move them to a local variable
std::queue<command_request> commands = std::move(m_commands);

while (commands.size() > 0) {
while (m_commands.size() > 0) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change would cause a regression since m_commands would be empty after moving it into the local variable

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If m_commands is empty after the move, then the loop is never entered. And this will always be the case, or am I wrong?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct but it would revert the fix introduced in 2a22c32

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants