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

Provide more available error messaging for nonexistent and invalid frames in canTransform #187

Merged
merged 2 commits into from
Nov 18, 2019

Conversation

emersonknapp
Copy link
Contributor

  • Make error messaging available to callers and provide it in more cases such as for frames that don't exist
  • Combine warnFrameId with validateFrameId, allow it to configurably choose between throwing exceptions, passing back error messages, and logging error messages
  • Update the uses of these functions and remove fully redundant validation calls - ones that were called twice for seemingly no gain, and potentially involved multiple tf tree walks where one would have sufficed

Second half of follow-through for #130
Part of fix for #118
Part of https://github.com/ros-security/aws-roadmap/issues/74

Signed-off-by: Emerson Knapp emerson.b.knapp@gmail.com

tf2/src/buffer_core.cpp Outdated Show resolved Hide resolved
@@ -853,15 +860,19 @@ bool BufferCore::canTransform(const std::string& target_frame, const std::string
if (target_frame == source_frame)
return true;

if (warnFrameId("canTransform argument target_frame", target_frame))
std::unique_lock<std::mutex> lock(frame_mutex_);
Copy link
Contributor

Choose a reason for hiding this comment

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

what are you locking here?
It's preferable locking in scopes to make this clear:

{
  std::unique_lock<std::mutex> lock(frame_mutex_);
  // ...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I.e. could you validate w/o locking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line is in the original, it's locking reading of the tf frame tree until the end of the function, for validateFrameId and canTransformNoLock. It shows up as a new line here because it was moved a few lines up.

Copy link
Contributor

@thomas-moulard thomas-moulard Oct 23, 2019

Choose a reason for hiding this comment

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

Is there a risk of validateFrameId to call back canTransform? If we expect a lock to be held before validateFrameId is called, shouldn't it be validateFrameIdNoLock to align with canTransformNoLock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not that validateFrameID has to have a lock held, but that we are pinning the two validation calls to the same version of the tf tree by acquiring the lock. This doesn't seem to be done in all places where two frames are looked up and then have their transforms checked, and the tree walk happens based on a TimePoint, so I don't think it's necessary. My next version only locks for the tree walk itself. Uploading momentarily

Copy link
Contributor

Choose a reason for hiding this comment

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

The new version looks better to me.

@emersonknapp emersonknapp force-pushed the bufcore-validate branch 2 times, most recently from ee0fc37 to 4f4dca1 Compare October 24, 2019 21:18
@emersonknapp emersonknapp changed the title Provide more complete validation for canTransform in BufferCore Provide more available error messaging for nonexistent and invalid frames in canTransform Oct 24, 2019
@emersonknapp
Copy link
Contributor Author

@tfoote @rotu ping for review

tf2/src/buffer_core.cpp Outdated Show resolved Hide resolved
@emersonknapp emersonknapp force-pushed the bufcore-validate branch 2 times, most recently from f4e2ffd to 1ec9d3e Compare November 15, 2019 00:12
…n doing canTransform lookups

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Copy link
Contributor Author

@tfoote @allenh1 I've updated it with the faster string concatenation method and rebased onto the upstream - PTAL

@tfoote tfoote merged commit f7027f9 into ros2:ros2 Nov 18, 2019
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.

None yet

6 participants