Re: [boost] [outcome] success-or-failure objects

Subject: Re: [boost] [outcome] success-or-failure objects
From: Miguel Ojeda (miguel.ojeda.sandonis_at_[hidden])
Date: 2018-01-25 00:57:54

On Thu, Jan 25, 2018 at 12:34 AM, Gavin Lambert via Boost
<boost_at_[hidden]> wrote:
> On 24/01/2018 19:33, Emil Dotchevski wrote:
>>
>> If it is a logic error for the caller to not expect this state, you have
>> no
>> idea what the caller will end up doing and you most definitely can't
>> expect
>> you can safely throw.
>
>
> You can always safely throw.  Sometimes it will have poor consequences (eg.
> leaking memory, leaving bad data, abandoning locks, aborting the process),
> but those are all well-defined consequences and are themselves side effects
> of poor coding practices that can be corrected.
>
> So that doesn't make any sense.  If you call r.error() on an r that doesn't
> have an error, there are four choices:
>
>   1. No checking.  This is performance optimised and leads to UB.  (In Ye
> Olde Outcome, it would have returned a garbage bit pattern that could cause
> all sorts of weird logic, though probably not any far-reaching harm due to
> the way error_code itself works.  In Outcome v2 it's most likely going to
> just return a default-initialised one, which is entirely harmless.)
>
>   2. Check and assert.  This aborts the process if you do something wrong.
> This is great for developer builds.  Not so great if it happens in
> production (due to insufficient testing).
>
>   3. Check and throw.  This will *also* abort the process if left uncaught,
> but otherwise admits the possibility of unwinding an operation and moving on
> to the next operation, which will perhaps not encounter the same problem (as
> if the problem were commonly exercised it would have been found sooner).
>
>   4. Check and return.  This makes it a defined behaviour of the method and
> thus isn't a logic error any more, so doesn't need further consideration.
>
> #1 has the potential to ruin everybody's data and corrupt all the things.
> But its faster as long as *all* callers obey the rules.
>
> #2 is "abandon ship!" mode.  It's a good default behaviour for most
> applications, but is often an over-reaction to things that were prevented
> before they caused memory corruption.  Worse, if implemented using assert()
> then it will devolve to #1 in release builds.
>
> #3 also defaults to "abandon ship!" mode, but allows for the possibility of
> a controlled shutdown or of abandoning a subtask and moving on, in the cases
> where tasks are reasonably separated and don't depend on each other.  (And
> even in cases where they do depend on each other, you can detect that a task
> depends on a task that failed and mark it as errored as well.)
>
> #3 is always the safest option.  #2 has gained some popularity because it
> allows quickly finding issues during development, but it has the wrong
> fallback behaviour -- it should fall back to #3 by default, and require
> explicit opt-in for #1 for areas identified as performance-critical in
> profiling (and have been extensively tested).
>
> #2 has also gained some popularity because of people abusing
> exceptions-as-control-flow and catching and ignoring all exceptions, which
> is itself a misuse of the tools.  But tools being misused does not
> necessarily mean they are bad tools.
>
> The right balance, I think, is to default to #2 in debug builds with a
> fallback to *#3* in release builds, then get code into a state where *no
> exceptions are ever actually thrown* (except when exercising unit tests, of
> course), including when run in production on real data.  Then, gradually, as
> and when *their callers* are proved safe, the checks can be omitted (#1) in
> the performance-critical paths only.
>

That can be a simple rule of thumb for many applications and errors.
However, it still depends on the application and the kind of error.

For example, there are applications where the risk of unintended
side-effects like "leaving bad data" after a logic error might not be
acceptable and therefore throwing "for cleaning up" might be deemed
too risky vs. just terminating. For other applications, there might be
security ramification as well, e.g. if you start unwinding when you
know your state is corrupted, you could actually be helping an
attacker rather than just dying.

Therefore, one can easily argue that in the case of logic errors the
rule of thumb should be to terminate, rather than to throw; unless you
have considered the risks very carefully. You can do the same
reasoning on the topic of uncaught exceptions.

Miguel