[LEAF] A sort of review

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[LEAF] A sort of review

Boost - Dev mailing list
Hi Everyone,
According to the schedule the review period of LEAF should have ended, but
since I haven't heard a clear announcement, I assume it isn't too late to
provide feedback.

I regret that I cannot afford to make a full review of the library that
Emil deserves. So this will be just "notes". I didn't have time to study
the implementation of the library. I only read most of the documentation
(documentation is really good), tried some toy examples to see if I can
brake it (didn't manage to), and participated in Boost discussions on the
subject. Only from this am I confident that the library has been well
designed. Its goals are clearly set and sound, and the designed explained
in the docs.

We have competing libraries in Boost for error handling (Boost.Outcome) but
they put accent on different things, and besides Boost is already known to
host competing designs and approaches to the same problem.

The need to transport arbitrary number of information collected at
different levels of call stack is a sound goal, and well addressed in LEAF.
The choice to put error data on the stack is great, and well understood and
explored, for instance in
https://www.research.ed.ac.uk/portal/files/78829292/low_cost_deterministic_C_exceptions_for_embedded_systems.pdf

I find function preload() (https://zajo.github.io/leaf/#tutorial-preload)
really great and novel. This is the first big C++ library that I know that
implements it. This really makes the error reporting code brief, clean and
bug-free.

Some people have reported that the usage of multiple lambdas in
try-catch-like statements is inconvenient. It is; but I think it works to
the advantage of the library. My approach to handling errors based on
skipping dependent operations (
https://akrzemi1.wordpress.com/2019/04/25/handling-errors-is-canceling-operations/)
is that you report errors (start the "stack unwinding") in many places,
keep the most functions error neutral, and you stop the "stack unwinding"
only in very few places. In this approach, only very few places in program
code would have to put these try-catch statements with lambdas, and the
clumsiness of the syntax would encourage the users to adapt the methodology
"stop unwinding in very few places".

My only criticism of the library is that it makes a serious design choice
to use a thread-local storage and fails to communicate it clearly to
potential users in the documentation. It is ok for the library to make the
choice to use TLS, but it is not ok to omit it from the "design choices" in
documentation. I wouldn't bring it up if the documentation was poor; but
because it is so high-standard, it strikes me that this information is
missing.

The reservations about TLS have nothing to do with performance. First, some
platforms do not provide it. But as Bjorn Reese indicated, the same
functionality could be implemented with a global lock-free map with thread
IDs as keys, and no TLS would be required. Second, and I think more
serious, is that thread-local solutions (including the global hash map with
thread IDs) play very badly with coroutines: in a coroutine world a
function can start executing in one thread, and finish executing in a
different one. All those TLS-based solutions compile and may pass tests but
are likely to fail with random results or crashes later. Many good
programmers do not realize this. Emil informs me that this problem could be
worked around by manually propagating some error information at the point
where coroutines are managed. And I trust him on that. But in order for the
programmer to do this manual part correctly, she has to know that this
problem exists, and that she has to do it. This has to be stated
prominently in the documentation along with the example how to do it.

This criticism is only about the documentation. Not the design or
implementation.

I would not use LEAF for production applications I work with. The reason is
that I am satisfied c++ exceptions and I find their cost tolerable in most
of the places. In the remaining parts, usually low-level, where the
performance or explicit control flows are important, I do not care about
the detailed information about the failure, other than an integer code. But
this is just my use case.

My familiarity with the domain: I report failures in my code :). I study
the topic of reporting failures effectively a lot. I follow the progress of
ISO C++ Committee on the subject. I have co-authored the documentation of
Boost.Outcome.

As I said, I didn't review the implementation, so the Review Manager will
consider it in weighing my opinion, but my recommendation is to **ACCEPT**
LEAF into Boost.

Thank you, Emil, for writing and sharing this library.

Regards,
&rzej;

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [LEAF] A sort of review

Boost - Dev mailing list
On Thu, Jun 4, 2020 at 1:57 AM Andrzej Krzemienski via Boost <
[hidden email]> wrote:
> I find function preload() (https://zajo.github.io/leaf/#tutorial-preload)
> really great and novel. This is the first big C++ library that I know that
> implements it. This really makes the error reporting code brief, clean and
> bug-free.

Thanks for the review.

I remember when Boost Exception was discussed (ancient history now), David
Abrahams pointed out that it is unfortunate that it requires an exception
to be caught before it can be augmented. In terms of Boost Exception, you
must say:

try
{
  f();
}
catch( boost::exception & e )
{
  e << more_info{};
  throw;
}

He thought that we're not really doing error handling here, so the addition
of more_info should happen automatically. Unfortunately there is no good
way to do this with Boost Exception. In that design, you need to interact
with the exception object and the only way to get a reference to it is to
catch it. Perhaps it's possible to do some extreme acrobatics to hide the
catch from the user (e.g. a macro), but at that point it becomes
questionable whether that would be desirable at all. Further, e <<
more_info{} requires dynamic memory to be allocated, which could fail. As
ugly as it is, it's best to spell it all out, so we know what's going on.

I never forgot his feedback though. The analogous LEAF code is this:

auto p = leaf::preload(more_info{}); // Does not allocate dynamic memory
f();

So, thank you Dave. :)

> My only criticism of the library is that it makes a serious design choice
> to use a thread-local storage and fails to communicate it clearly to
> potential users in the documentation.

This is specified here: https://zajo.github.io/leaf/#context. There are 8
places in the documentation that mention thread local.

Perhaps it could be more prominent, I'm open to suggestions. IMO this isn't
something most users need to worry about in 2020. If I were one of the few
users who must worry about it, I'd search for "thread_local" in the source
code to be sure.

> Emil informs me that this problem could be
> worked around by manually propagating some error information at the point
> where coroutines are managed. And I trust him on that.

Don't. :)

I don't know if it's possible, I don't know coroutines enough. From what
I've read, it should be possible to store a leaf::context in an awaitable,
and activate/deactivate it as the coroutine is resumed/suspended (I plan,
in fact, to rename context::activate/deactivate to resume/suspend).

Thanks once again.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [LEAF] A sort of review

Boost - Dev mailing list
Emil Dotchevski wrote:

> I remember when Boost Exception was discussed (ancient history now), David
> Abrahams pointed out that it is unfortunate that it requires an exception
> to be caught before it can be augmented. In terms of Boost Exception, you
> must say:
>
> try
> {
>   f();
> }
> catch( boost::exception & e )
> {
>   e << more_info{};
>   throw;
> }
>
> He thought that we're not really doing error handling here, so the
> addition of more_info should happen automatically. Unfortunately there is
> no good way to do this with Boost Exception. In that design, you need to
> interact with the exception object and the only way to get a reference to
> it is to catch it.

I kind of suspect it will be possible. We could preload the more_info{},
a-la LEAF, and then move that into the active exception "record" on
destruction, if that record is kept outside the exception object, but in a
side storage like a thread-local map.

Sort of a LEAF/Boost.Exception hybrid, where the boost::exception base only
contains the error_id like in LEAF, and the actual error_info is in the map.

The `preload` will need to allocate, but the destructor won't, which seems
enough for our purposes.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [LEAF] A sort of review

Boost - Dev mailing list
On Thu, Jun 4, 2020 at 3:10 PM Peter Dimov via Boost <[hidden email]>
wrote:
> Sort of a LEAF/Boost.Exception hybrid, where the boost::exception base
only
> contains the error_id like in LEAF, and the actual error_info is in the
map.

What is the benefit of keeping the error info in a map, rather than in a
context<> like LEAF does? Is it that you want to simplify the Boost
Exception base types?

One benefit of keeping it in the exception object is that transporting the
exception with the data between threads is trivial.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost