Skip to content

[Dashing] Avoid reference cycle between Context and GraphListener#1233

Closed
ivanpauno wants to merge 1 commit intodashingfrom
ivanpauno/dashing/topic-fix-not-delete-context
Closed

[Dashing] Avoid reference cycle between Context and GraphListener#1233
ivanpauno wants to merge 1 commit intodashingfrom
ivanpauno/dashing/topic-fix-not-delete-context

Conversation

@ivanpauno
Copy link
Copy Markdown
Member

Backport of #906.

This change breaks ABI of the GraphListener class.

That class is never used directly by an user.
In the internal places where that class is being used, it's being stored as a shared_ptr.
i.e.: ABI of other objects shouldn't be affected, and thus the user shouldn't be affected.

I'm not quite sure if we should backport a fix like this or not as it's hard to verify if it actually breaks "user" ABI or not.
If the answer is no, I will delete this from Dashing and Eloquent patch release board.

* Use weak_ptr to store context in graph listener

Signed-off-by: Barry Xu <Barry.Xu@sony.com>
@ivanpauno ivanpauno added bug Something isn't working in review Waiting for review (Kanban column) labels Jul 20, 2020
@ivanpauno ivanpauno self-assigned this Jul 20, 2020
@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Jul 20, 2020

I would decline to do this, even with the fair points about the class not being used externally and therefore not a high risk ABI break.

Is someone asking for this fix or we just think it's good to have?

@ivanpauno
Copy link
Copy Markdown
Member Author

Is someone asking for this fix or we just think it's good to have?

I think nobody is asking for it.
I've added it to the board in the past, and I wanted to double check what we should do in these cases.

I will close and delete it from the Dashing backports board.
If someone specifically asks for this in the future we can reconsider making the backport.

@ivanpauno ivanpauno closed this Jul 20, 2020
@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Jul 20, 2020

Ok, yeah perhaps someone on our team would disagree, but I'd prefer to document the issue rather than break ABI, in this case.

Perhaps here, if appropriate: https://index.ros.org/doc/ros2/Releases/Release-Dashing-Diademata/#known-issues

@ivanpauno
Copy link
Copy Markdown
Member Author

Ok, yeah perhaps someone on our team would disagree, but I'd prefer to document the issue rather than break ABI, in this case.

Perhaps here, if appropriate: https://index.ros.org/doc/ros2/Releases/Release-Dashing-Diademata/#known-issues

Sounds like a good idea, I've opened ros2/ros2_documentation#826 to document the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working in review Waiting for review (Kanban column)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants