Skip to content

Fix bug when mixing shared_ptr and bind.#470

Merged
mjcarroll merged 1 commit intomasterfrom
bugfix/hanging_lifecycle_test
May 4, 2018
Merged

Fix bug when mixing shared_ptr and bind.#470
mjcarroll merged 1 commit intomasterfrom
bugfix/hanging_lifecycle_test

Conversation

@mjcarroll
Copy link
Copy Markdown
Member

@mjcarroll mjcarroll commented May 4, 2018 •
edited
Loading

It looks like using the std::shared_ptr in the std::bind was causing the refcount to get incremented, so you end up with a dependency loop, and the object never gets properly cleaned up.

Fixes #469

@mjcarroll mjcarroll requested a review from Karsten1987 May 4, 2018 03:13
@mjcarroll mjcarroll added the in progress Actively being worked on (Kanban column) label May 4, 2018
@mjcarroll
Copy link
Copy Markdown
Member Author

mjcarroll commented May 4, 2018 •
edited by mikaelarguedas
Loading

  • Linux (bionic) Build Status

(mikaelarguedas) Other platforms just in case:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@mjcarroll mjcarroll self-assigned this May 4, 2018
@mjcarroll mjcarroll added bug Something isn't working in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels May 4, 2018
@mikaelarguedas mikaelarguedas mentioned this pull request May 4, 2018
Merged
@mikaelarguedas
Copy link
Copy Markdown
Member

mikaelarguedas commented May 4, 2018

lgtm, I'll leave it up to @Karsten1987 provide the final review/approval

Karsten1987
Karsten1987 approved these changes May 4, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

feel free to merge. I still can't really explain why this was the cause that only fastrtps tests were hanging, but it seems to fix it. So lgtm.

@mjcarroll mjcarroll merged commit 1be4d2d into master May 4, 2018
@mjcarroll mjcarroll deleted the bugfix/hanging_lifecycle_test branch May 4, 2018 18:10
@mjcarroll mjcarroll removed the in review Waiting for review (Kanban column) label May 4, 2018
@mikaelarguedas mikaelarguedas mentioned this pull request May 4, 2018
Closed
31 tasks
nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022
@hidmic
[YAML Parser] Depend on rcutils only (ros2#470)
f9ceef5 
* Make rcl_yaml_param_parser depend on rcutils only.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

* Address peer review comments.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022
@dirk-thomas
add wait for closed file handles on Windows (ros2#470)
3732a24 
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@Karsten1987 Karsten1987 Karsten1987 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mjcarroll mjcarroll

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mjcarroll @mikaelarguedas @Karsten1987