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

Avoid a reference cycle between Node and TimeSource #488

Merged
merged 2 commits into from
Jan 6, 2020
Merged

Avoid a reference cycle between Node and TimeSource #488

merged 2 commits into from
Jan 6, 2020

Conversation

ivanpauno
Copy link
Member

@ivanpauno ivanpauno added enhancement New feature or request in review Waiting for review (Kanban column) labels Jan 3, 2020
@ivanpauno ivanpauno self-assigned this Jan 3, 2020
dirk-thomas
dirk-thomas reviewed Jan 3, 2020
View reviewed changes
sloretz
sloretz reviewed Jan 3, 2020
View reviewed changes
rclpy/rclpy/time_source.py
if node is None:
raise RuntimeError('Attached node has gone out of scope before expected')
return node
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

I would think this case is worth raising for (trying to do an operation that requires a node, but the time source is not attached to one), and the other case of the weak reference being invalid is expected and not worth raising for.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed the code to not raise an error in case the weak reference is invalid.
The code previous to this change was working when self._node was None, so I don't think that an error shold be raised in that case.

@ivanpauno
Address peer review comments
a9fdca3 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
dirk-thomas
dirk-thomas approved these changes Jan 6, 2020
View reviewed changes
Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

LGTM with passing CI.

@mjcarroll
Copy link
Member

Let's make sure to include the ros2* test suite in this CI, to see if issues with nightly hangs are resolved (specifically ros2topic and ros2action)

@ivanpauno ivanpauno mentioned this pull request Jan 6, 2020
Merged
@ivanpauno
Copy link
Member Author

ivanpauno commented Jan 6, 2020 •
edited
Loading

Let's make sure to include the ros2* test suite in this CI, to see if issues with nightly hangs are resolved (specifically ros2topic and ros2action)

The hangs observed in CI might be related with rclpy object destruction order problems.
This PR together with #490 might "fix" the problems.
But the value of this PR is completly unrelated to whatever those tests hang or not, so I'm just going to include rclpy in the CI run.

CI up to rclpy, only fastrtps:

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

@ivanpauno
Copy link
Member Author

The windows fail is an unrelated flaky test, here this same branch passing:

  • Windows Build Status

I'm merging this!

@ivanpauno ivanpauno merged commit dee0645 into master Jan 6, 2020
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/avoid-node-time-source-refcycle branch January 6, 2020 21:13
@ivanpauno ivanpauno mentioned this pull request Feb 20, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirk-thomas dirk-thomas dirk-thomas approved these changes

@sloretz sloretz Awaiting requested review from sloretz sloretz is a code owner

Assignees

@ivanpauno ivanpauno

Labels
enhancement New feature or request in review Waiting for review (Kanban column)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ivanpauno @mjcarroll @dirk-thomas @sloretz