return NULL when conversion fails for any reason

[dirk-thomas]

WIthout the patch the conversion function continue after setting a Python error. With the patch it return NULL instead.

• Linux
• Linux-aarch64
• macOS
• Windows

Ready for review.

[wjwwood]

lgtm

[mikaelarguedas]

would it make sense to do the same (returning a NULL pointig PyObject) when the conversion fails the other way around?
Don't know if it's a problem you faced and wanted to address in this fix or if you're tidying up error management in general.

this is just a question, the patch looks good to me as is

[dirk-thomas]

would it make sense to do the same (returning a NULL pointing PyObject) when the conversion fails the other way around?

I don't understand what you mean with "the other way around" and why "return a NULL pointing PyObject". Can you please clarify.

Don't know if it's a problem you faced and wanted to address in this fix or if you're tidying up error management in general.

I found this while debugging the case in ros2/rclpy#95 and tried to fix the error handling.

[mikaelarguedas]

Oh I miss that related PR.

I don't understand what you mean with "the other way around" and why "return a NULL pointing PyObject". Can you please clarify.

By "the other way around" I meant when we fail to convert from C to Python.

In several places we assert that the result is valid (including in this function for the other types without error checking, e.g. PyFloat_AS_DOUBLE). These asserts would disappear in the Release builds and the code will keep going resulting on crashes down the road. I haven't experienced any crashes related to this so have been delaying rewriting the error management in this file.

But that's totally unrelated to this PR I was just wondering what would be the good way to handle this in the future when we decide to focus effort on fixing the error handling.

[dirk-thomas]

By "the other way around" I meant when we fail to convert from C to Python.

Good point. I updated the patch to handle that direction too.

In several places we assert that the result is valid (including in this function for the other types without error checking, e.g. PyFloat_AS_DOUBLE). These asserts would disappear in the Release builds and the code will keep going resulting on crashes down the road. I haven't experienced any crashes related to this so have been delaying rewriting the error management in this file.

I think the assert's are onlty there as a sanity check. The field setter should already prevent wrong types to sneak in. Therefore I think that is ok as-is.

[dirk-thomas]

See ros2/rclpy#96 for checking for a valid return value.

• Linux
• Linux-aarch64
• macOS
• Windows