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.

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

Fix clock state cached time to be a copy, not a reference. #2092

Merged
merged 1 commit into from
Jan 27, 2023

Conversation

clalancette
Copy link
Contributor

That is, in cache_last_msg(), we were just taking a shared_ptr reference. While this is pretty fast, it also means that any changes to that message would be reflected internally. Instead, make a new shared pointer out of that message when it comes in, which essentially causes this to be a copy.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

This should fix #2088 . FYI @fujitatomoya and @mjcarroll .

That is, in cache_last_msg(), we were just taking a shared_ptr
reference.  While this is pretty fast, it also means that
any changes to that message would be reflected internally.
Instead, make a new shared pointer out of that message when
it comes in, which essentially causes this to be a copy.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@mjcarroll
Copy link
Member

LGTM wit CI!

@clalancette
Copy link
Contributor Author

CI:

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

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

@clalancette lgtm, let me try this fixes the flaky test since i can reproduce this issue my local environment. i want to make sure about this.

@clalancette
Copy link
Contributor Author

@clalancette lgtm, let me try this fixes the flaky test since i can reproduce this issue my local environment. i want to make sure about this.

Yeah, that would be good to verify. It fixed it for me here, but I will note that test_time_source has another flaky bug in it that I need to track down (in the callbacks test).

@fujitatomoya
Copy link
Collaborator

@clalancette i tried 100 times and no failure observed on that test. thanks for the fix.

I will note that test_time_source has another flaky bug in it that I need to track down (in the callbacks test).

yeah, i am aware of that, i will post the issue so that we cannot miss it, lets track this with another issue.

on this PR, good to go with green CI 🚀

@clalancette
Copy link
Contributor Author

@clalancette i tried 100 times and no failure observed on that test. thanks for the fix.

Thanks for double-checking!

yeah, i am aware of that, i will post the issue so that we cannot miss it, lets track this with another issue.

Sounds good. I'll open one later today.

@clalancette
Copy link
Contributor Author

CI is green here, so merging this in. Thanks for the reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🧑‍🌾 Flaky test: test_time_source.parameter_activation
3 participants