-
Notifications
You must be signed in to change notification settings - Fork 417
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[foxy] Remove finalization of guard_condition from GraphListener::__shutdown() #1401
Conversation
Thanks for bringing this up @brawner ! Yes, this looks pretty wrong: the guard condition is stored in a I think that what makes things a complicated is that The other option is to return a Both options are API breaking, so between the two I prefer the first one. If we want a non-API breaking fix, I don't see another option than leaking like proposed here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to hear @wjwwood opinion before this gets merged, but it looks ok (it is the only backportable change I see possible).
I'm working on backporting things to foxy so this technically blocks #1383. How about I retarget this to foxy and then a more appropriate change can be applied to rolling? I like moving things to a shared_ptr for get_interrupt_guard_condition because everything else is memory managed in Context. |
👍 I think that's fine |
f9f860f
to
127505e
Compare
Signed-off-by: Stephen Brawner <brawner@gmail.com>
127505e
to
e21e62c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm, assuming you'll do something different in rolling. This is just leaking the guard condition right?
That's correct. I'll make a PR for rolling and see how you two like it.
I'll try something like Ivan's first suggestion.
|
Thanks for the reviews. The failures in the Fpr job are unrelated to this PR and will probably be resolved in #1383, which does not show the same failures. |
Hi, we noticed that the memory leak mentioned by @brawner in the post is still present in the Foxy branch! We have been able to reproduce it in a simple test file that repeatedly calls |
IIRC, we didn't find an ABI compatible fix. |
The unit test
error_run_graph_listener_destroy_context
(copied below) was added in #1330 which tests destroying a context before callingrun()
on the graph_listener (and ultimately shutdown). This was leading to a memory read error in theshutdown_guard_condition_
when trying to finalize it. A special case was added in #906, which would try to finalize this guard_condition in the event that the context was already destroyed.This addresses #1391
However, I'm not sure if this PR is the correct solution. In #906, this situation was specifically called out in (#906 (comment)) and its reply.
@wjwwood
@ivanpauno
In fact, there is a memory leak in the
error_run_graph_listener_destroy_context
test with this PR (but no memory errors).Signed-off-by: Stephen Brawner brawner@gmail.com