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.

Sign up for GitHub

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

Error handling in callbacks #88

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Error handling in callbacks #88

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 20 additions & 8 deletions src/ZenohUTransport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,16 +108,16 @@ v1::UAttributes ZenohUTransport::attachmentToUAttributes(
.deserialize<std::vector<std::pair<std::string, std::string>>>();

if (attachment_vec.size() != 2) {
spdlog::error("attachmentToUAttributes: attachment size != 2");
// TODO: error report, exception?
throw std::invalid_argument("invalid attachment size");
}

if (attachment_vec[0].second.size() == 1) {
if (attachment_vec[0].second[0] != UATTRIBUTE_VERSION) {
spdlog::error("attachmentToUAttributes: incorrect version");
// TODO: error report, exception?
throw std::invalid_argument("incorrect UAttribute version");
}
};
} else {
throw std::invalid_argument("incorrect UAttribute version size");
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
v1::UAttributes res;
res.ParseFromString(attachment_vec[1].second);
return res;
Expand Down Expand Up @@ -194,7 +194,11 @@ v1::UStatus ZenohUTransport::registerRequestListener_(
zenoh::detail::loan(query));

query_map_.emplace(std::move(id_str), std::move(cloned_query));
listener(queryToUMessage(query));
try {
listener(queryToUMessage(query));
} catch (const std::exception& e) {
spdlog::error("query processing failed: {}", e.what());
Comment on lines +199 to +200
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
};

auto on_drop = []() {};
Expand Down Expand Up @@ -223,7 +227,11 @@ v1::UStatus ZenohUTransport::registerPublishNotificationListener_(
// NOTE: listener is captured by copy here so that it does not go out
// of scope when this function returns.
auto on_sample = [this, listener](const zenoh::Sample& sample) mutable {
listener(sampleToUMessage(sample));
try {
listener(sampleToUMessage(sample));
} catch (const std::exception& e) {
spdlog::error("sample processing failed: {}", e.what());
sashacmc marked this conversation as resolved.
Show resolved Hide resolved
}
};

auto on_drop = []() {};
Expand Down Expand Up @@ -261,7 +269,11 @@ v1::UStatus ZenohUTransport::sendRequest_(const std::string& zenoh_key,
const auto& sample = reply.get_ok();
spdlog::debug("resp_callback: {}",
sample.get_payload().deserialize<std::string>());
resp_callback(sampleToUMessage(sample));
try {
resp_callback(sampleToUMessage(sample));
} catch (const std::exception& e) {
spdlog::error("sample processing failed: {}", e.what());
sashacmc marked this conversation as resolved.
Show resolved Hide resolved
}
spdlog::debug("resp_callback: done");
} else {
spdlog::error(
Expand Down