Ensure compliant publisher API #210

hidmic · 2020-07-24T16:44:44Z

Precisely what the title says. Connected to ros2/rmw#252.

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

hidmic · 2020-07-24T16:48:18Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

-  return destroy_publisher(publisher);
+
+  rmw_ret_t inner_ret = destroy_publisher(publisher);
+  if (RMW_RET_OK != inner_ret) {


I don't like having to do this to ensure the first error that occurs is the one that pops up and that no subsequent error goes unnoticed.

If we had a way to tell rcutils that we're in error handling or finalization/cleanup mode so that it doesn't complain about error states being overwritten (and prints the new one to stderr, or aborts the program, or whatever), this code would simpler (and a bit more neat). What do reviewers think?

The current logic isn't super nice, but isn't wrong either.
Maybe directly logging all destruction errors and setting a generic error state at the end is a better idea.

The current logic isn't super nice, but isn't wrong either.

No, it's not incorrect, but it's... messy.

Maybe directly logging all destruction errors and setting a generic error state at the end is a better idea.

We could do that, but that doesn't cover errors during cleanup. Besides, it's important to know what the actual error was to do something about it (or debug it), nevermind leaks. At any rate, I don't mind doing this. I just figured it'd much easier if rcutils took care of redirecting all RCUTILS_SET_ERROR_MSG() for a given scope. @clalancette @wjwwood @jacobperron for feedback too.

We could do that, but that doesn't cover errors during cleanup

Do you mean the cleanup that a function does when it fails?
In that case it would be good to set an error message about what failed, and log all cleanup failures.
So logging is fine in that case too,

I just figured it'd much easier if rcutils took care of redirecting all RCUTILS_SET_ERROR_MSG() for a given scope

How?

For instance, having a flag in TLS that client code can toggle. For C++ implementations, we may be able to use RAII for easier control of the error handling scope. If done right it can reduce clutter.

Ok, that's fair. As I said elsewhere, I wonder about the usefulness of a return code if we log everything.

The return code would still be useful, as the caller can know that sth failed (it doesn't matter if we return the first error code or the last one).
The error string won't be useful if we log everything, yes.

I wonder about the usefulness of a return code if we log everything

Isn't the purpose of the return code so that the calling function knows something went wrong, and optionally if there is enough information in the return code to then make an intelligent decision about what to do about it? The logging is more for after-the-fact debugging so having everything logged doesn't seem bad to me.

Having an error string would also still be useful for when logging is turned down, I think.

Why not simply pass an RMW error state object around inside the implementation, with a setter that implements the behaviour of returning the first error and logging the rest? That's basically the pattern you have here with ret and inner_ret, but it'd remove the messiness from the code.

and optionally if there is enough information in the return code to then make an intelligent decision about what to do about it?

IMHO that's the whole point of having different, documented return codes, vs. a bool.

Why not simply pass an RMW error state object around inside the implementation, with a setter that implements the behaviour of returning the first error and logging the rest?

Yeah, I was trying not to (re)build this behavior for each RMW implementation, but to make it widely available. I'll probably push this as-is until we've reached consensus on how to do this nicely.

ivanpauno · 2020-07-24T16:59:32Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

@@ -1954,9 +1987,16 @@ extern "C" rmw_publisher_t * rmw_create_publisher(
        static_cast<void *>(&msg),
        nullptr))
    {
+      rmw_error_state_t error_state = *rmw_get_error_state();


nit: Inverting the logic in L1978, setting an error message and returning NULL will make the logic a bit clearer.

ivanpauno · 2020-07-24T17:07:22Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

-  return destroy_publisher(publisher);
+
+  rmw_ret_t inner_ret = destroy_publisher(publisher);
+  if (RMW_RET_OK != inner_ret) {


The current logic isn't super nice, but isn't wrong either.
Maybe directly logging all destruction errors and setting a generic error state at the end is a better idea.

eboasson

It looks correct to me.

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

hidmic · 2020-07-27T19:34:06Z

CI up test_rmw_implementation against all RMW implementations:

Linux
Linux-aarch64
macOS
Windows

ivanpauno

LGTM

hidmic · 2020-07-28T21:46:56Z

Ok, CI's green and reviewers are happy. Going in!

hidmic · 2020-07-28T22:03:07Z

Confirmed locally that this patch introduces no new leaks. valgrind does point out that we may be leaking through create_message_typesupport. We can follow-up in a separate PR.

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

hidmic added 2 commits July 24, 2020 13:43

Ensure compliant publisher construction/destruction.

bc001a9

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

Ensure compliant publisher QoS queries.

fb55e32

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

hidmic requested review from eboasson and ivanpauno July 24, 2020 16:44

hidmic commented Jul 24, 2020

View reviewed changes

ivanpauno approved these changes Jul 24, 2020

View reviewed changes

eboasson approved these changes Jul 27, 2020

View reviewed changes

Address peer review comments.

dc55691

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

hidmic mentioned this pull request Jul 27, 2020

Simplify error handling in failure mode or during finalization ros2/rcutils#269

Open

hidmic requested a review from ivanpauno July 28, 2020 20:50

ivanpauno approved these changes Jul 28, 2020

View reviewed changes

hidmic merged commit 2e2c6d1 into master Jul 28, 2020

delete-merged-branch bot deleted the hidmic/compliant-publisher-impl branch July 28, 2020 21:47

ahcorde pushed a commit that referenced this pull request Oct 8, 2020

Ensure compliant publisher API (#210)

4c1b4f0

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

ahcorde pushed a commit that referenced this pull request Oct 15, 2020

Ensure compliant publisher API (#210)

398812c

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure compliant publisher API #210

Ensure compliant publisher API #210

hidmic commented Jul 24, 2020

hidmic Jul 24, 2020 •

edited

Loading

ivanpauno Jul 24, 2020 •

edited

Loading

hidmic Jul 24, 2020

ivanpauno Jul 24, 2020

hidmic Jul 24, 2020 •

edited

Loading

hidmic Jul 24, 2020

ivanpauno Jul 24, 2020

gbiggs Jul 25, 2020

eboasson Jul 27, 2020

hidmic Jul 27, 2020

ivanpauno Jul 24, 2020

ivanpauno Jul 24, 2020 •

edited

Loading

eboasson left a comment

hidmic commented Jul 27, 2020 •

edited

Loading

ivanpauno left a comment

hidmic commented Jul 28, 2020

hidmic commented Jul 28, 2020

Ensure compliant publisher API #210

Ensure compliant publisher API #210

Conversation

hidmic commented Jul 24, 2020

hidmic Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

ivanpauno Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanpauno Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

eboasson left a comment

Choose a reason for hiding this comment

hidmic commented Jul 27, 2020 • edited Loading

ivanpauno left a comment

Choose a reason for hiding this comment

hidmic commented Jul 28, 2020

hidmic commented Jul 28, 2020

hidmic Jul 24, 2020 •

edited

Loading

ivanpauno Jul 24, 2020 •

edited

Loading

hidmic Jul 24, 2020 •

edited

Loading

ivanpauno Jul 24, 2020 •

edited

Loading

hidmic commented Jul 27, 2020 •

edited

Loading