Error handling using std::error_code and the proposed std::expected type #801

cryptocode · 2018-04-14T21:37:34Z

Docs and rationale: https://github.com/cryptocode/raiblocks/wiki/Error-handling-in-the-node-using-std::error_code-and-std::expected

This PR applies the above error handling ideas to a handful of RPC handlers. A few functions with the double-response-write problem are fixed as well, using std::error_code.

I've put a few error codes in a common enum, which can be used elsewhere later on.

decode_unsigned is used to test std::expected which allows both values and errors to be returned. I think expected is a good fit for the node, but it's probably a good idea to introduce it gradually to see how well things compose.

Since expected isn't standardized yet (expected in C++20), I've included expected-light (a single-header file) which we'll remove once compilers ships with a std implementation.

PlasmaPower

Some more general thoughts:

It looks like you didn't transform some RPC functions, like block_create. Was there a reason for this?
Should we return an error code from the RPC functions?
Should we use something similar to the Rust try macro (now an operator) for preconditions, and maybe other places?
When std::expected is converted to a boolean, it returns true if it was successful. There's not much we can do about this, but it is the opposite of our previous idiom of errors being truthy.
Should we keep the success-first pattern in if statements? Personally I favor putting the error first so it's right next to the failing condition.

Overall I really like the direction of this PR.

PlasmaPower · 2018-05-14T23:53:28Z

rai/node/rpc.cpp

 	std::string account_text;
+


Nit: accidental whitespace change?

Yeah, fixed.

PlasmaPower · 2018-05-14T23:54:12Z

rai/node/rpc.cpp

 	rai::block_hash hash;
 	auto head_str (request.get_optional<std::string> ("head"));
 	rai::transaction transaction (node.store.environment, nullptr, false);
 	if (head_str)
 	{
-		error = hash.decode_hex (*head_str);
-		if (!error)
+		if (hash.decode_hex (*head_str))


Nit: this condition should be inverted if we're going with the success-first pattern.

See my main comment about this topic.

PlasmaPower · 2018-05-15T00:04:02Z

rai/node/rpc.cpp

-	if (destinations_text.is_initialized ())
+
+	auto destinations (decode_unsigned (request.get<std::string> ("destinations", "0")));
+	if (!ec && !destinations)


Nit: in an above function (I don't remember which) you don't check ec when parsing future values. It doesn't matter that much, but we should probably be consistent.

Agreed, though I don't seem to find the missing check.

cryptocode · 2018-05-15T09:12:44Z

It looks like you didn't transform some RPC functions, like block_create. Was there a reason for this?

Yeah, only a few functions are converted as I wanted to set up a path for using std::expected and get that reviewed. There's a lot of actions to be converted and tested properly.

Serg offered to help with the conversion once the framework is in place.

If you prefer this PR to do the full conversion of RPC instead of multiple PR's, I'm happy to do that, though it'll take some time for me to get around to that.

Should we return an error code from the RPC functions?

I was thinking that process_request can't do much about them anyway since the error response is already sent by the action handler.

One option would be to let process_request do the error response, but doing it in the action handler allows for some useful idioms, see end of blocks for an example.

Should we use something similar to the Rust try macro (now an operator) for preconditions, and maybe other places?

That would be great. P0779R0 is a proposal to implement this, which will work with std::expected. Not sure what a good C++ idiom is right now. There's also a monadic extension proposal to allow chaining of expected.

When std::expected is converted to a boolean, it returns true if it was successful. There's not much we can do about this, but it is the opposite of our previous idiom of errors being truthy.

Since std::expected is not an error, but a value or an error, I think it makes sense.

I see your point, though - it's going to be a bit painful to read the code base as the old idiom gets slowly replaced.

Should we keep the success-first pattern in if statements? Personally I favor putting the error first so it's right next to the failing condition.

I have no strong opinions here. Possibly the no-early-returns principle in the code base affected the decision to place errors at the end, since you're then forced to nest statements ?

PlasmaPower · 2018-05-16T14:34:08Z

rai/node/rpc.cpp

-		boost::optional<std::string> modified_since_text (request.get_optional<std::string> ("modified_since"));
-		if (modified_since_text.is_initialized ())
+		auto count (decode_unsigned (request.get<std::string> ("count", ""), std::numeric_limits<uint64_t>::max ()));
+		if (!count)


Here's the condition that should have a !ec if we're going with that style.

Ah, good catch, I'll add that.

PlasmaPower · 2018-05-16T14:37:05Z

That would be great. P0779R0 is a proposal to implement this, which will work with std::expected. Not sure what a good C++ idiom is right now.

Should we make our own macro for the time being?

cryptocode · 2018-05-16T14:45:43Z

The proposal requires language changes. A decent C-style macro solution would require statement expressions, which is not supported by MSVC. Unless someone has come up with an alternative trick that I'm not aware of. Do you have any ideas on how to solve this properly on C++14?

PlasmaPower · 2018-05-16T15:44:58Z

Oh, I forgot that statements aren't expressions in C++ (unlike in Rust 😉). We could probably make a macro that takes the name of a variable and sets the value, but that doesn't seem like a good idea.

cryptocode · 2018-06-06T07:23:04Z

I need std::expected/std::error_code in the IPC/API code now so I'll introduce it there. The RPC changes can be rebased on top of that later.

cryptocode force-pushed the std-expected-rpc branch from f0f793a to f8eab00 Compare April 19, 2018 17:57

cryptocode force-pushed the std-expected-rpc branch from f8eab00 to 616da17 Compare May 13, 2018 20:48

PlasmaPower reviewed May 15, 2018

View reviewed changes

cryptocode force-pushed the std-expected-rpc branch 2 times, most recently from 034269a to 334b243 Compare May 15, 2018 10:35

PlasmaPower reviewed May 16, 2018

View reviewed changes

cryptocode force-pushed the std-expected-rpc branch 2 times, most recently from 2c2825e to 9f8e10b Compare May 16, 2018 21:22

Error handling using std::error_code and the proposed std::expected type

3d45038

cryptocode force-pushed the std-expected-rpc branch from 9f8e10b to 3d45038 Compare May 17, 2018 00:05

cryptocode closed this Jun 6, 2018

cryptocode mentioned this pull request Jul 2, 2018

std::error_code and std::expected implementation #942

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error handling using std::error_code and the proposed std::expected type #801

Error handling using std::error_code and the proposed std::expected type #801

cryptocode commented Apr 14, 2018

PlasmaPower left a comment •

edited

Loading

PlasmaPower May 14, 2018

cryptocode May 15, 2018

PlasmaPower May 14, 2018

cryptocode May 15, 2018

PlasmaPower May 15, 2018

cryptocode May 15, 2018

cryptocode commented May 15, 2018 •

edited

Loading

PlasmaPower May 16, 2018

cryptocode May 16, 2018

cryptocode May 16, 2018

PlasmaPower commented May 16, 2018

cryptocode commented May 16, 2018 •

edited

Loading

PlasmaPower commented May 16, 2018

cryptocode commented Jun 6, 2018

Error handling using std::error_code and the proposed std::expected type #801

Error handling using std::error_code and the proposed std::expected type #801

Conversation

cryptocode commented Apr 14, 2018

PlasmaPower left a comment • 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

cryptocode commented May 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PlasmaPower commented May 16, 2018

cryptocode commented May 16, 2018 • edited Loading

PlasmaPower commented May 16, 2018

cryptocode commented Jun 6, 2018

PlasmaPower left a comment •

edited

Loading

cryptocode commented May 15, 2018 •

edited

Loading

cryptocode commented May 16, 2018 •

edited

Loading