Hi all,
Thanks for the feedback. Reading between the lines, it sounds like we are not
quite happy with the flavor of the approach. To deconstruct, so we can evaluate
each axis:
1. Canonical indication of pass/fail (as opposed to implicit rules about
return value being negative or positive or nonzero indicating failure)
2. Coupling of control flow with result reporting Bubble-up functionality -
Essentially a convention for handling die()-like functionality without
calling die() and ending the process
3. Further error context via breadcrumbs (Foo errored because Bar errored
because Baz errored)
4. Error codes - Rather than just -1, having a richer, named space of errors
5. Rich error data - void *data attached to each error (not actually proposed
as part of promise but may be relevant)
6. Async-capable - shelving this since it may require further justification.
First, bubble-up functionality (#3) seems like it will be critical to support if
we intend to stop calling die(). This can be done either exhaustively (by
manually checking each error to see if it is fatal, and if so, invoking a
control-flow action), or by a macro. I couldn’t find any established conventions
for doing this in C, and the original patch series doesn’t really address this
well either since it requires a manual `if (...)` check, so it is an area for
further exploration. (Actually another proposed alternative was `longjmp`, but
it sounds like that is a can of worms to be avoided.)
Back to the overall approach. To contrast to one alternative, error reporting
functions a la the intriguing thread by Peff:
https://lore.kernel.org/git/20160927191955.mympqgylrxhkp24n@xxxxxxxxxxxxxxxxxxxxx/
With that pattern, there is no concept of an overall status, either pass or
fail. One must assume that if there was an error, then the operation failed, and
if no error, then it passed. But then there is a question of whether an error
was fatal. We know that some errors are fatal (meaning an immediate halt of
execution is in order) and others are not. If we implemented this approach we
would probably need to add this bool is_fatal as a field to struct
error_context.
Also, unless a macro is employed, the reporting of errors is not coupled with
control flow (#2) unlike promise, since PROMISE_THROW() invokes a return
statement. Of course, Peff’s code there was just a sketch instead of a
full-blown patch, but not coupling the two could easily lead to bugs in the form
of accidentally-omitted return statements, so perhaps a macro is in order if
that pattern is implemented.
The lifecycle of anything in void *data poses its own challenges. While promise
offers macros to handle the strbuf release at EOL, special handlers would need
to be written to free anything stored in void *data. Which brings me to this
feedback:
> 1) It is hard to see how we can wrap the return value of the function in a
> promise and preserve type safety. Even if we used some kind of union the
> compiler will not warn us if the caller reads a different member to the one
> that the callee set.
>
> 2) It obscures the return type of the function and forces callers to unpack
> the value from the promise adding complexity to the calling code.
I should clarify that the promise concept is not intended to expand in scope as
far as the return value. It should remain as `int` and only ever `int`, so no
return value would be anything other than a simple number, meaning AFAIK (1)
should not apply. For (2), if auxiliary data structures (additional ints, or
structs, etc.) are outputs, they would need to be via "output parameters" passed
into the function. Those output parameters should not need to be unpacked, since
their type is preserved in the normal way.
However to return to void *data in struct error_context, the unpacking *would*
be necessary since *data doesn’t have a concrete type. Therefore, if we go with
struct error_context, the *data field should be omitted since it adds complexity
without need.
Since we are dropping async support, I am going to rename the topic from
`Promise` to `Result` as well, since the idea is equivalent to `Result<T, E>` in
Rust, aka `Either a b` in Haskell.
Regarding error messages, totally valid criticism that they are confusing to the
user as posed in the original patchset. Eventually perhaps the messages could be
reworded to increase clarity, but to avoid scope creep, let’s just not show
them. On the topic of scope, I agree with what I believe Phillip Wood implied,
that excessive refactoring should not be required to adopt this pattern. As such
I will keep this in mind in the next iteration.
There is more to discuss about strings vs error codes and the future of error
codes as well as memory allocation, but this is probably enough for now. I may
be interested to try out the error_context as well as a concept next just to see
what it would be like, but do people agree with the differential analysis so far
and changes proposed? And curious if you have any other thoughts?
- Philip
On Wed, Feb 21, 2024 at 1:03 PM Jeff King <peff@xxxxxxxx> wrote:
>
> On Tue, Feb 20, 2024 at 01:19:16PM +0100, Patrick Steinhardt wrote:
>
> > While we're already at it throwing ideas around, I also have to wonder
> > whether this would be a long-term solution towards computer-friendly
> > errors. One of the problems we quite frequently hit in Gitaly is that we
> > are forced to parse error messages in order to figure out why exactly
> > something has failed. Needless to say, this is quite fragile and also
> > feels very wrong.
> >
> > Now if we had a more structured way to pass errors around this might
> > also enable us to convey more meaning to the caller of Git commands. In
> > a hypothetical world where all errors were using an explicit error type,
> > then this error type could eventually become richer and contain more
> > information that is relevant to the calling script. And if such rich
> > error information was available, then it would not be that far fetched
> > to ask Git to emit errors in a computer-parsable format like for example
> > JSON.
>
> I think what I'm proposing (and if I understand correctly what Phillip
> was thinking) is somewhat orthogonal. I agree that structured errors are
> nice for computers to read. But it does open up a pretty big can of
> worms regarding classifying each error, especially as root causes are
> often multi-level.
>
> For example, imagine that the caller asks to resolve a ref. We might get
> a syscall error opening the loose ref. Or we might get one opening the
> packed-refs file (in a reftable world, you might imagine errors opening
> various other files). What is the structured error? Obviously it is "we
> can't resolve that ref" at some level. But the caller might want to know
> about the failed open (whether it is just ENOENT, or if we ran into
> EPERM or even EIO).
>
> Or looking at higher levels; if I ask for the merge base between A and
> B, but one of those can't be resolved, how do we communicate that error?
> It is some sort of "cannot resolve" error, but it needs to be
> parameterized to know which is which.
>
> All of those questions can be answered, of course, but now we are
> developing a micro-format that lets us describe all errors in a
> standardized way. And I think that is going to put a burden on the code
> which is generating the errors (and potentially on the consumers, too,
> if they have to decipher the structure to figure out what they want).
>
> Whereas what I was really advocating for is punting on the structured
> thing entirely. We keep our unstructured string errors for the most
> part, but we simply let the caller pass in a context that tells us what
> to do with them. That lets us keep providing specific error messages
> from low-level functions without printing to stderr or exiting, which
> higher-level code (especially lib-ified code) would not want.
>
> I think it could also be the first building block for making more
> structured errors (since those low-level callers are free to provide
> lots of details), but it doesn't have to be.
>
> -Peff
