Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[dashing backport] add mutex in add/remove_node and wait_for_work to protect concurrent use/change of memory_strategy_ #857

Merged
merged 1 commit into from Sep 20, 2019
Merged

[dashing backport] add mutex in add/remove_node and wait_for_work to protect concurrent use/change of memory_strategy_ #857

merged 1 commit into from Sep 20, 2019

Conversation

zmichaels11
Copy link
Contributor

Signed-off-by: Zachary Michaels zmichaels11@gmail.com

@dirk-thomas @zmichaels11
add mutex in add/remove_node and wait_for_work to protect concurrent …
0b58d56 
…use/change of memory_strategy_ (#837)

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Copy link
Contributor Author

zmichaels11 commented Sep 19, 2019 •
edited by tfoote

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

@dirk-thomas
Copy link
Member

Please use the prefix "[dashing backport] " in the title to make it obvious that this is a backport. Also please reference the ticket which is being backported here.

@zmichaels11 zmichaels11 changed the title add mutex in add/remove_node and wait_for_work to protect concurrent use/change of memory_strategy_ [dashing backport] add mutex in add/remove_node and wait_for_work to protect concurrent use/change of memory_strategy_ Sep 19, 2019
@zmichaels11
Copy link
Contributor Author

Backporting PR #837 to Dashing
@dirk-thomas Should the backported PR also be added to the title?

@dirk-thomas
Copy link
Member

Should the backported PR also be added to the title?

No, the PR number doesn't get converted into a reference when it is in the title.

@tfoote
Copy link
Contributor

tfoote commented Sep 19, 2019

@zmichaels11 All the CI's failed on this. the linux one looks like jenkins lost connection during the build so I restarted it. But all the others look to have failed real failures.

@zmichaels11
Copy link
Contributor Author

I'm looking into why its failing locally.

@tfoote
Copy link
Contributor

tfoote commented Sep 19, 2019

I think you have the same problem that you're testing it against master not dashing https://github.com/ros2/ros2/blob/dashing/ros2.repos

@zmichaels11
Copy link
Contributor Author

It looks like that fixed the other PR.
Trying the same fix to this one; rerunning with dashing:

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

@zmichaels11
Copy link
Contributor Author

@tfoote it looks like it was the same problem; all builds are passing

@dirk-thomas
Copy link
Member

@ros-pull-request-builder retest this please

@dirk-thomas
Copy link
Member

@zmichaels11 It seems that the PR build has repeatedly the same test failure which doesn't appear in the dev job so it might be related to this change.

@zmichaels11
Copy link
Contributor Author

@dirk-thomas The tests fail due to spin_until_future_complete returning TIMEOUT.
It passes on my machine if I use the blocking variant of this call. I'm opening up a PR for changes to this test.

@zmichaels11
Copy link
Contributor Author

Possible fix for this PR: #862

@tfoote tfoote mentioned this pull request Sep 20, 2019
Closed
@mjcarroll mjcarroll mentioned this pull request Sep 20, 2019
Closed
tfoote
tfoote approved these changes Sep 20, 2019
View reviewed changes
Copy link
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

After looking around a little it seems that this is an existing issue that the PR jobs now hitting more often. ros2/build_farmer#184

It's come up in several other dashing backports such as #799 to that end I'm going to merge as is since the CI is passing and that the behavior has been seen on the dev jobs as well indicates that it's not a regression caused by this change.

@tfoote tfoote merged commit 6e76503 into ros2:dashing Sep 20, 2019
@tfoote tfoote added this to Released in Dashing Patch Release 4 Sep 20, 2019
@zmichaels11 zmichaels11 deleted the zmichaels11/backport-#837 branch September 23, 2019 17:34
nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022
@clalancette
refactor for removing unnecessary source code (ros2#857)
c07cee4 
* refactor for removing unnecessary source code

Co-authored-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tfoote tfoote tfoote approved these changes

@dirk-thomas dirk-thomas Awaiting requested review from dirk-thomas

@hidmic hidmic Awaiting requested review from hidmic

Assignees
No one assigned
Labels
None yet
Projects
No open projects
Dashing Patch Release 4
Released
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@zmichaels11 @dirk-thomas @tfoote