Error handling in callbacks #88
Conversation
|
If we parsing the attachments to generate UMessage failed, how can we know what is the right callback to call? The change you propose sends and internal error but there is no source or sink so we don't know what to do with the error. I say we log and drop the message for now |
sashacmc
force-pushed
the
error_handling
branch
from
7c959d6 to
82f10e1
Compare
Ok, I reverted changes to only report error in the log |
| } | ||
| }; | ||
| } else { | ||
| throw std::invalid_argument("incorrect UAttribute version size"); |
There was a problem hiding this comment.
The error handling model we have been using for up-cpp requires that exceptions only be thrown in a situation where the caller has done something unrecoverable (i.e. passed an invalid UUri to an interface) - exceptions are a panic situation and are not expected to be caught. Faults on the network or with incoming messages should not throw an exception as those faults are transient and the developer of the entity could not have reasonably accounted for this - a bad actor on the network should be ignored.
These should not be exceptions. Maybe we need a way of reporting bad incoming messages to an upper layer, but that would be done in another way (e.g. via callback that receives a UStatus)
There was a problem hiding this comment.
Zenoh-cpp also can throw the exception if something going wrong (ZException inherited from the std::exception) e.g. from deserialize. So there is nothing wrong with catching them together.
But yes, if we have a hard rule not to produce any exceptions zenoh-cpp allows to pass a parameter with a return code, then check it, pass it to the calling function, and so on. But IMHO it will be more cumbersome.
| } catch (const std::exception& e) { | ||
| spdlog::error("query processing failed: {}", e.what()); |
There was a problem hiding this comment.
What exceptions are possible here? We generally don't want to blanket catch all exceptions as they can mask programmatic mistakes. This should either catch specific exceptions that represent temporary failures (that is, failures where trying again may produce a different result), or catch nothing at all.
Additionally, shouldn't a non-ok status be returned if an exception is caught here? Doesn't that represent a failure that needs to be communicated upward?
There was a problem hiding this comment.
What exceptions are possible here? We generally don't want to blanket catch all exceptions as they can mask programmatic mistakes. This should either catch specific exceptions that represent temporary failures (that is, failures where trying again may produce a different result), or catch nothing at all.
deserialization problem etc.
Additionally, shouldn't a non-ok status be returned if an exception is caught here? Doesn't that represent a failure that needs to be communicated upward?
I had a question about this and there was a suggestion in the previous version of this PR, but the listener as it currently stands doesn't allow it. So, by advice from this comment #88 (comment), I just log and drop the message.
|
Hi @gregmedd, I'm still waiting your response, should we rework all error handling to avoid exception usage? |
Added error handling for message/sample conversions (including exceptions thrown by Zenoh) in callbacks