... and now it compiles in godbolt: https://godbolt.org/z/xPMzP5 (I've also made some changes to the code, to better demonstrate the possible error-handling paths) Comments/corrections anyone? Thanks Ronen On Thu, Jul 4, 2019 at 5:34 PM Ronen Friedman <rfriedma@redhat.com> wrote:
Hi Kefu,
Like I said in the meeting: I really think this is the right direction. I am not sure I fully understand the specifics you've proposed. Here is my take (which may be exactly what you meant):
Suppose this is the "happy path": [image: outcome_1.jpg]
In the face of an error we may want to have this behavior (1'st option): [image: outcome_2.jpg]
- let's assume that the values of type T are wrapped in outcome::outcome<T, error-code, exception-code> (I'd rather using that over outcome::result); - Thus, f1() should be written to return the wrapper type (outcome<T1>), - f2() is now a function from outcome<T1> to outcome<T2>. - as we may want to have error codes "pass thru" f2(), without requiring f2() to be modified with the boiler-plat code for the pass-thru, let us wrap f2() into pass_thru<T1,T2>(f2). I have attached code that presents one way of doing just that.
note that while f2() is just a "pass thru" for an incoming error code, we still need to "re-code" the error value in the incoming outcome<T1> into the same value in outcome<T2>.
Another desired behaviour, as you mention in the email, is to split the logic into a separate "happy days" and "error path":
[image: outcome_3.jpg]
We can use a separate wrapper around the two paths (note that both paths are expected to return optional<T2>).
Here is some code that demonstrates what I mean (compiled with g++9, but the link in Godbolt is only as a display mechanism):
https://godbolt.org/z/i-d4TU <- use the new link above, not this one
Note that the code is not polished yet. For example: I think I would be able to replace the call
return WpassR<Oint, Ofloat>(of2_ps, x)
with
return WpassR(of2_ps, x)
using some magic (deduction guides? that will require building some structs around) - if we think this direction is worth our time.
Ronen
On Wed, Jul 3, 2019 at 7:13 AM kefu chai <tchaikov@gmail.com> wrote:
hi guys,
i just came across boost::outcome[0]. it reminded me the discussion we had back in Barcelona regarding to the error handling in crimson. well, strictly speaking, it's not limited to errors. it covers the non-error handling as well.
the question is: shall we start prototyping the crimson variant of outcome<> now? if yes, probably we can leverage boost::outcome<>?
a little bit background:
seastar uses exception for propagating the error. but it incurs runtime overhead. because, to throw an exception, the libstdc++ runtime needs to acquire a global lock.
well, some of us might want to argue, why not just return a future<Result, Error>? let me use an example here, imagine we are handling a write request in OSD. we might need to go through following steps:
1. perform some sanity tests, for instance, to see if the OSD is ready for handling the write request 2. try to read the object info of the object from local storage to see if it already exists 3. write to the object to the local storage, and send write requests to replica OSDs (assuming it's in a replicated pool), wait for the completions of these write ops. 4. update the statistics 5. reply to the client
and it's intuitive to structure these steps using chained continuation like
do_with(std::move(request), [this](auto request) { return perform_tests(request->object_id).then([request, this] { return read_object_info(request->object_id); }).then([request, this](optional<object_info> object_info) { return when_all( write_local(request->object_id, request->offset, request->data), parallel_for_each(replica_osds, [request](auto replica_osd) { return replica_osd->write_remote(request->object_id, request->offset, request->data) })); }).then([write_size=request.data.size(), this] { update_statistics(write_size); return reply_to(reply_t::success, request); }); }).handle_exception([](auto exception) { return reply_to(reply_t::failure, exception.error_code, request); });
in which, if any test fails in step#1, we either need to wait until the OSD is ready, or just need to bail out, and skip the following steps. the "handle_exception()" clause is used to handle the "bail out" case, where we cannot do anything to serve the request. for instance, the request is invalid.
we want to differentiate two types of errors. one of them are actually exceptions which does not happen often in real world, and we don't need/want to optimize for this case. but the other case could be normal. for instance, it's fairly normal that an object does not exist yet, when we are trying to write to it. and we do want to be performant when handling these "errors" in this category, and also, we want to do this in a convenient way just like handling exceptions.
because, we need an efficient way to convey the message to caller that "please skip the following continuations, and i would go to this handling route instead". if my memory serves me correctly, we think that we need to create a wrapper around seastar::future<> to allow the caller to do something like
// a helper to run func or skip it template<typename Func> auto ignore_on_error(Func&& f) { return [f=std::move(f)](auto&& t) { return t.is_value() ? f(t.value()) ? t; } }
return read_object_info(oid).then( return ignore_on_error([](object_info& oi) { return handle_write_with_object_info(std::move(oi)); }).then([](auto t) { return handle_write_without_object_info(); }); );
in the example above, i assume we will do something very different depending on if the object's existence.
cheers,
--- [0] https://www.boost.org/doc/libs/1_70_0/libs/outcome/doc/html/index.html
-- Regards Kefu Chai
