Skip to content
This repository was archived by the owner on May 31, 2025. It is now read-only.

Use boost::shared_ptr to fix memory leak in BagPlayer#1373

Merged
mikepurvis merged 1 commit intoros:melodic-develfrom
efernandez:fix_bag_player_memory_leak
Apr 23, 2018
Merged

Use boost::shared_ptr to fix memory leak in BagPlayer#1373
mikepurvis merged 1 commit intoros:melodic-develfrom
efernandez:fix_bag_player_memory_leak

Conversation

@efernandez
Copy link
Copy Markdown
Contributor

@efernandez efernandez commented Apr 22, 2018

This fixes two memory leaks in BagPlayer by changing the std::map of RAW pointers to BagCallback to smart pointers boost::shared_ptr<BagCallback>"

  1. On destruction, b/c the destructor doesn't delete the RAW pointers in the std::map.
  2. In register_callback is called with the same topic name more than once w/o calling unregister_callback for that topic name.

This should also work now if we call unregister_callback for a topic not previously registered. Before that would delete a null pointer.

with std::map of BagCallback, instead of RAW pointer.

This solves two memory leaks:
1. On destruction, b/c the destruction doesn't delete the RAW pointers
   in the std::map.
2. In register_callback is called with the same topic name more than
   once w/o calling unregister_callback for that topic name.
@efernandez
Copy link
Copy Markdown
Contributor Author

FYI @dirk-thomas

}

void BagPlayer::unregister_callback(const std::string &topic) {
delete cbs_[topic];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This means it will now not be deleted until the whole object goes out of scope. Perhaps there should still be a cbs_.erase(topic) or something here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Of course, that's in the next line. I didn't remove it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh lol, so I see.

@efernandez
Copy link
Copy Markdown
Contributor Author

Looks like the tests are currently broken b/c of something else, unrelated with this PR, right?

@mikepurvis
Copy link
Copy Markdown
Member

Yes, the debian stretch issue is being investigated in #1358; the other test is flakey.

@mikepurvis mikepurvis merged commit 2d1382a into ros:melodic-devel Apr 23, 2018
@efernandez efernandez deleted the fix_bag_player_memory_leak branch April 23, 2018 21:12
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.

2 participants