Errors should never pass silently: Fatal errors.

[Torsten Bronger]

Hallöchen!

There are errors that you cannot handle since the rest of the
program makes no sense if the error occurs, or because they indicate
a bug. Thus, I have a function like

func PanicIfError(err error) {
if err != nil {
logger.Panic(err)
}
}

and I call this function very often in my code. I've never seen
this idiom anywhere though, in fact, I think the Go code I've seen
so far ignores many errors which should not occur. But coming from
Python, I stick to the "errors should never pass silently" matra.

So ... is this PanicIfError function okay? Or is there a better
way?

Regards,
Torsten.

--
Torsten Bronger Jabber ID: torsten...@jabber.rwth-aachen.de

[Konstantin Khomoutov]

On Wed, 25 May 2016 19:51:59 +0200
Torsten Bronger <bro...@physik.rwth-aachen.de> wrote:

> Hallöchen!
>
> There are errors that you cannot handle since the rest of the
> program makes no sense if the error occurs, or because they indicate
> a bug. Thus, I have a function like
>
> func PanicIfError(err error) {
> if err != nil {
> logger.Panic(err)
> }
> }
>
> and I call this function very often in my code. I've never seen
> this idiom anywhere though, in fact, I think the Go code I've seen
> so far ignores many errors which should not occur. But coming from
> Python, I stick to the "errors should never pass silently" matra.
>
> So ... is this PanicIfError function okay? Or is there a better
> way?

I'm afraid you are victim of the "exceptions" mentality (which is quite
understandable since you have Python background). Since Go appeared as
a systems-programming language, it considers errors to be just that --
errors which has to be handled. Say, your program failed to open a
file. Is this a fatal error? I hear you say "yes". But what if that
program is an SMTP or IMAP server and it hit the "no space left on
device" condition while trying to create a file to save the data about
to be supplied by a connected client? Would you prefer your SMTP
server to crash with a nice exception stack trace in the log file or
to return the «451, "4.3.0", Mail server temporarily rejected message.»
error to that client and continue working -- rejecting incoming mails
until the server admins fix the situation? I bet you'd prefer the
latter. Actually, that's how professionally-written software works.

Sure, there is another case. There is another class of software which
I'm tempted to call "script-like" or "batch-like": it performs a
sequence of actions, and usually when any of them fails, the rest has
no sense to be performed. In this case crashing and burning on an
error is just okay.

There's also the third case: things like dereferencing nil pointers and
accessing unowned memory (and receiving SIGSEGV/SIGBUS from the OS).
Those are really exceptional cases and must be handled using panic().
But the runtime already does this for you.
A similar case is checking whether certain requirements on the values
of the arguments of a function which is a part of the public API of
your package are fullfilled. Say, if your function has an argument of
type int, and it only can sensibly work if that value is >= 2, then by
all means go on check if that invariant holds and panic() if it doesn't.

Please contrast the examples I presented: handling an error from the OS
and handling a programmer's error -- they are really different cases.

[Torsten Bronger]

Hallöchen!

Konstantin Khomoutov writes:

> [...]
>
> [...] Say, your program failed to open a file. Is this a fatal

> error? I hear you say "yes".

I have accepted that Go is not an everything-is-exception language
like Python and try to write ideomatically. (But note that Python
distinguishes between exception and fatal error.)

My program is only a few hundred lines long yet, but I have already
three cases where I can only abort:

- The DB behaves inconsistently.
- json.Marshal() fails.
- http.ResponseWriter.Write() fails.

And all of this occurs in multiple places in the code. I could wrap
the three cases in own functions that also contain the panic, but I
would do this only to avoid my ubiquitous PanicIfError. Should I do
this wrapping nevertheless, or keep the PanicIfError's?

[Manlio Perillo]

Il giorno mercoledì 25 maggio 2016 19:53:51 UTC+2, Torsten Bronger ha scritto:

Hallöchen!

There are errors that you cannot handle since the rest of the
program makes no sense if the error occurs, or because they indicate
a bug.  Thus, I have a function like

    func PanicIfError(err error) {
            if err != nil {
                    logger.Panic(err)
            }
    }

and I call this function very often in my code.  I've never seen
this idiom anywhere though, in fact, I think the Go code I've seen
so far ignores many errors which should not occur.

Usually, when a function detects an error, it should either:

- pass it to the caller, unchanged

- pass it to the caller, with additional context

- handle it

Handling the error may also mean to terminate a program (what you say is correct, it is better to fail early then to continue the program in an inconsistent state), but often this decision should be left to the high level layers (e.g. the main function).

> [...]

Manlio 

[Konstantin Khomoutov]

On Wed, 25 May 2016 21:37:19 +0200
Torsten Bronger <bro...@physik.rwth-aachen.de> wrote:

[...]
> Say, your program failed to open a file. Is this a fatal
> > error? I hear you say "yes".
>
> I have accepted that Go is not an everything-is-exception language
> like Python and try to write ideomatically. (But note that Python
> distinguishes between exception and fatal error.)
>
> My program is only a few hundred lines long yet, but I have already
> three cases where I can only abort:
>
> - The DB behaves inconsistently.

Depends on how do you define "inconsistently".

If you detect that some _invariant_ held by your program, and related
to the state kept in the database, fails, this means you have an error
somewhere so that the state became corrupt or some other code messed
with that state. I think if this is your case, panic() is a sensible
thing to do. If you merely mean the DB errored out on you, the sql.DB
instance will try hard to get you a working connection.

> - json.Marshal() fails.

Depends on whether you're sure it _must_ succeed. That is, if its
outcome may depend on the data supplied by users, you must not panic()
on this function failing.

I'd say this is akin to the MustCompile() function of the regex
package: it's OK to panic if you're sure your input data is OK.
That is, it's about asserting the invariant "my data is surely OK".

> - http.ResponseWriter.Write() fails.

Really? It will fail for, say, network glitch between the client and
your server. Certainly not the case to panic(). Possibly log the
problem, tear the connection down and continue serving requests.

> And all of this occurs in multiple places in the code. I could wrap
> the three cases in own functions that also contain the panic, but I
> would do this only to avoid my ubiquitous PanicIfError. Should I do
> this wrapping nevertheless, or keep the PanicIfError's?

If you want to panic why not just panic where you think it's OK to do?

[Sam Vilain]

On 5/25/16 12:37 PM, Torsten Bronger wrote:
> My program is only a few hundred lines long yet, but I have already
> three cases where I can only abort:
>
> - The DB behaves inconsistently.
> - json.Marshal() fails.
> - http.ResponseWriter.Write() fails.
>
> And all of this occurs in multiple places in the code. I could wrap
> the three cases in own functions that also contain the panic, but I
> would do this only to avoid my ubiquitous PanicIfError. Should I do
> this wrapping nevertheless, or keep the PanicIfError's?

I prefer to use log.Fatal for these "too hard to handle now" errors;
that way you can wrap the error with useful context, so at least when
the program aborts it aborts with a more useful error message than a
panic stack dump.

Sam

[aro...@gmail.com]

On Wednesday, May 25, 2016 at 12:59:58 PM UTC-7, Konstantin Khomoutov wrote:

On Wed, 25 May 2016 21:37:19 +0200
Torsten Bronger <bro...@physik.rwth-aachen.de> wrote:

[...]

> - http.ResponseWriter.Write() fails.

Really?  It will fail for, say, network glitch between the client and
your server.  Certainly not the case to panic().  Possibly log the
problem, tear the connection down and continue serving requests.

It will fail a lot more often than just random network glitches.  If the client hangs up before reading the entire response then Write() will fail.  For example, a user mashing refresh on a page a few times can cause a request to be generated and aborted, even for pretty small and fast responses.

I agree with Konstantin: Of the three listed conditions (db, json, write), only json.Marshal seems reasonable when used analogously to regex.MustCompile.  DB errors are similar to the http failures.  Even if the query must always succeed according to the scheme, the connection to the DB can fail temporarily (timeout, too many connections, network glitch) and while it may make perfect sense to fail the current request, it doesn't mean that the server should crash, IMO.

- Augusto

[Dave Cheney]

The log package supports a Fatal function, that sounds like the right fit. But please don't ever use it in a a library that someone else may consume.

[Torsten Bronger]

Hallöchen!

Konstantin Khomoutov writes:

> On Wed, 25 May 2016 21:37:19 +0200
> Torsten Bronger <bro...@physik.rwth-aachen.de> wrote:
>
> [...]
>>

>> My program is only a few hundred lines long yet, but I have
>> already three cases where I can only abort:
>>
>> - The DB behaves inconsistently.
>
> Depends on how do you define "inconsistently".
>
> If you detect that some _invariant_ held by your program, and related
> to the state kept in the database, fails, this means you have an error
> somewhere so that the state became corrupt or some other code messed
> with that state.

I mean this.

> [...]

>
>> - json.Marshal() fails.
>
> Depends on whether you're sure it _must_ succeed.

It is "my" object, so if it fails, there is a bug.

> [...]
>
>> - http.ResponseWriter.Write() fails.
>
> Really?

No, here I was wrong. I thought in Django terms, where the HTTP
body is actually sent after my function returns.

> [...]

>
>> And all of this occurs in multiple places in the code. I could
>> wrap the three cases in own functions that also contain the
>> panic, but I would do this only to avoid my ubiquitous
>> PanicIfError. Should I do this wrapping nevertheless, or keep
>> the PanicIfError's?
>
> If you want to panic why not just panic where you think it's OK to
> do?

Right, I'll do so.

Thanks!

Tschö,

[Torsten Bronger]

Hallöchen!

Sam Vilain writes:

> On 5/25/16 12:37 PM, Torsten Bronger wrote:
>
>> My program is only a few hundred lines long yet, but I have already
>> three cases where I can only abort:
>>
>> - The DB behaves inconsistently.
>> - json.Marshal() fails.
>> - http.ResponseWriter.Write() fails.
>>
>> And all of this occurs in multiple places in the code. I could
>> wrap the three cases in own functions that also contain the
>> panic, but I would do this only to avoid my ubiquitous
>> PanicIfError. Should I do this wrapping nevertheless, or keep
>> the PanicIfError's?
>
> I prefer to use log.Fatal for these "too hard to handle now"
> errors;

PanicIfError also writes to the log. In which respect does
log.Fatal do more than that? Besides, I find a stack trace quite
informative.

Tschö,

[Torsten Bronger]

Hallöchen!

aro...@gmail.com writes:

> On Wednesday, May 25, 2016 at 12:59:58 PM UTC-7, Konstantin Khomoutov wrote:
>>

>> On Wed, 25 May 2016 21:37:19 +0200, Torsten Bronger wrote:
>>
>> [...]
>>
>>> - http.ResponseWriter.Write() fails.
>>
>> Really? It will fail for, say, network glitch between the client
>> and your server. Certainly not the case to panic(). Possibly
>> log the problem, tear the connection down and continue serving
>> requests.
>>
>
> It will fail a lot more often than just random network glitches.
> If the client hangs up before reading the entire response then
> Write() will fail. For example, a user mashing refresh on a page
> a few times can cause a request to be generated and aborted, even
> for pretty small and fast responses.

I was wrong here. This is not a case for a panic. (I found a
replacement for this case, though: If gocql.RandomUUID fails, I
cannot recover. It means that /dev/random is broken.)

> I agree with Konstantin: Of the three listed conditions (db, json,
> write), only json.Marshal seems reasonable when used analogously
> to regex.MustCompile. DB errors are similar to the http failures.
> Even if the query must always succeed according to the scheme, the
> connection to the DB can fail temporarily (timeout, too many
> connections, network glitch) and while it may make perfect sense
> to fail the current request, it doesn't mean that the server
> should crash, IMO.

Yes, this must not happen. I hope to get middleware running that
recovers and converts to 500'ers.

However, if the DB seems to be inconsistent, a human must look at it
before anything else is done with it IMO.

Tschö,

[Manlio Perillo]

Il giorno mercoledì 25 maggio 2016 23:15:04 UTC+2, Sam Vilain ha scritto:

> [...]

I prefer to use log.Fatal for these "too hard to handle now" errors;
that way you can wrap the error with useful context, so at least when
the program aborts it aborts with a more useful error message than a
panic stack dump.

Do not use log.Fatal when you don't know how to handle an error.

If you don't know how to handle an error, do not handle it.

Only when there is no other layer that can handle the error (e.g. the main package), you should call log.Fatal.

Manlio 

[Manlio Perillo]

Il giorno giovedì 26 maggio 2016 17:06:30 UTC+2, Torsten Bronger ha scritto:

Hallöchen!
> [...]

I was wrong here.  This is not a case for a panic.  (I found a
replacement for this case, though: If gocql.RandomUUID fails, I
cannot recover.  It means that /dev/random is broken.)

You may not be able to recover, but the code that calls your API *may be* able to recover.

Do not call panic, if you receive such an error in a non main package.

Manlio

[Konstantin Khomoutov]

On Thu, 26 May 2016 16:54:11 +0200
Torsten Bronger <bro...@physik.rwth-aachen.de> wrote:

> >> My program is only a few hundred lines long yet, but I have already
> >> three cases where I can only abort:
> >>
> >> - The DB behaves inconsistently.
> >> - json.Marshal() fails.
> >> - http.ResponseWriter.Write() fails.
> >>
> >> And all of this occurs in multiple places in the code. I could
> >> wrap the three cases in own functions that also contain the
> >> panic, but I would do this only to avoid my ubiquitous
> >> PanicIfError. Should I do this wrapping nevertheless, or keep
> >> the PanicIfError's?
> >
> > I prefer to use log.Fatal for these "too hard to handle now"
> > errors;
>
> PanicIfError also writes to the log. In which respect does
> log.Fatal do more than that? Besides, I find a stack trace quite
> informative.

Well, the original intent of panic() is to handle truly exceptional
cases, and "exceptional" here also means you did not actually _expect_
such condition to happen. The best example is dereferencing a nil
pointer: when you write your program you do not consciously attempt to
write code which explicitly dereferences nil pointers. Hence when such
dereferencing happens, you have no idea what could have gone wrong, and
that's why panic() helps you by sort-of snapshotting the executing
state for you (printing out stack traces is really just a more modern
way of dumping core files).

Conversely, when your program flow _explicitly_ detects a grave
("no way to handle") error, that's not an unexpected condition: at the
point the error is detected, the program flow has full awareness about
the context of the error. Hence there's little sense in snapshotting
the execution state because you can just log an error message decorated
with the description of its context and bring the program down.
That's what log.Fatal*() do.

If you'll think about this a bit more, you'll notice that well-known
ubiquitous programs (think of, say, GNU coreutils) do not blow up with
stack traces on errors -- they output an error and quit. Quite often
they quit with different exit code for different classes of errors
(say, configuration errors, input data errors, data output errors etc),
though in my practice the only occurence where custom exit codes were
useful was writing a program run by Sendmail for the final delivery --
because Sendmail is actually able to interpret a dozen of specific exit
codes of its delivery agents to make a decision on how to report the
failure to the client and what to do with the message.
log.Fatal*() always exit with code 1 but providing a custom "log and
exit" function is trivial. Conversely, blowing up with exception stack
trace dumping on innocent errors like the user passing a non-existing
command-line option is an approach popularized by sloppy coding in
languages/runtimes with exceptions.

Having said that, I admit that using panic() when log.Fatal*() appears
to be a good fit is not actually bad. Given your previous concrete
example, IMO panicking on json.Marshal() erroring out or detecting the
database state inconsistency is OK. Still, you should understand that
conceptually the stack trace won't give you any more clue to the case
of that error than you already have when it has happened.

[Torsten Bronger]

Hallöchen!

I first did this -- the respective function had an error return
value. Which meant that I had "if ... panic" three-liners all over
the place. And the callers are HTTP handlers with no return value.

If I used the package for other things, I would refactor it. But
until then, I keep it more succinct.

(Don't get me wrong -- I started with Go in January and I really
enjoy it and try to stick to the best common practises.)

[Torsten Bronger]

Hallöchen!

Konstantin Khomoutov writes:

> [...]
>

> [...] Still, you should understand that conceptually the stack

> trace won't give you any more clue to the case of that error than
> you already have when it has happened.

I find it extremely useful to know on which path the code came into
the erroneous state.

[Tamás Gulácsi]

Use gopkg.in/errgo to decorate your errors!

[Augusto Roman]

Or https://github.com/palantir/stacktrace

On Thu, May 26, 2016 at 9:22 AM Tamás Gulácsi <tgula...@gmail.com> wrote:

Use gopkg.in/errgo to decorate your errors!

--
You received this message because you are subscribed to a topic in the Google Groups "golang-nuts" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-nuts/DOjQrhBhssQ/unsubscribe.
To unsubscribe from this group and all its topics, send an email to golang-nuts...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Augusto Roman]

On Thu, May 26, 2016 at 8:56 AM Torsten Bronger <bro...@physik.rwth-aachen.de> wrote:

Hallöchen!

Manlio Perillo writes:

> Il giorno giovedì 26 maggio 2016 17:06:30 UTC+2, Torsten Bronger ha scritto:
>
>> [...]
>>
>> I was wrong here.  This is not a case for a panic.  (I found a
>> replacement for this case, though: If gocql.RandomUUID fails, I
>> cannot recover.  It means that /dev/random is broken.)
>
> You may not be able to recover, but the code that calls your API
> *may be* able to recover.  Do not call panic, if you receive such
> an error in a non main package.

I first did this -- the respective function had an error return
value.  Which meant that I had "if ... panic" three-liners all over
the place.  And the callers are HTTP handlers with no return value.

If I used the package for other things, I would refactor it.  But
until then, I keep it more succinct.

From experience, one thing that can happen when overusing panics is that you go from an ostensibly reasonable start:

  func Handler(w http.ResponseWriter, r *http.Request) {

    foo := db.GetFoo() // Might panic, but recovery middleware will take care of it.

    json.NewEncoder(w).Encode(foo)

  }

...to an unexpected crash of the entire server:

  func Handler(w http.ResponseWriter, r *http.Request) {

    // For speed, do several lookups in parallel:

    foo, bar := make(chan Foo), make(chan Bar)

    go func() { foo <- db.GetFoo() }()  // kaboom

    go func() { bar <- db.GetBar() }()

    json.NewEncoder(w).Encode(Baz{foo,bar})

  }

When this occurs right in the handler, it's the simple case.  It can easily occur further down the change inside of GetFoo if GetFoo() requires multiple DB calls and someone decides to speed those up by running them in parallel.

- Augusto

 

(Don't get me wrong -- I started with Go in January and I really
enjoy it and try to stick to the best common practises.)

Tschö,
Torsten.

--
Torsten Bronger    Jabber ID: torsten...@jabber.rwth-aachen.de

[Manlio Perillo]

Il giorno giovedì 26 maggio 2016 17:56:27 UTC+2, Torsten Bronger ha scritto:

Hallöchen!

Manlio Perillo writes:

> Il giorno giovedì 26 maggio 2016 17:06:30 UTC+2, Torsten Bronger ha scritto:
>
>> [...]
>>
>> I was wrong here.  This is not a case for a panic.  (I found a
>> replacement for this case, though: If gocql.RandomUUID fails, I
>> cannot recover.  It means that /dev/random is broken.)
>
> You may not be able to recover, but the code that calls your API
> *may be* able to recover.  Do not call panic, if you receive such
> an error in a non main package.

I first did this -- the respective function had an error return
value.  Which meant that I had "if ... panic" three-liners all over
the place.  And the callers are HTTP handlers with no return value.

What I do is to write, in the main package, wrapper functions that call log.Fatal on errors that I do not plan to handle.

As an example of a wrapper for http.Get for a console application:

func get(url string) *http.Response {

    resp, err := http.Get(url)

    if err != nil {

        log.Fatalf(....)

    }

    if resp.StatusCode != http.StatusOK {

        log.Fatalf(....)

    }

    return resp

}

> [...]

Manlio 

[Manlio Perillo]

Isn't this exactly the same as "blowing up with exception stack 

trace dumping on innocent error"?

Manlio

[Augusto Roman]

stacktrace decorates errors similar to errgo, but has an interface that makes more sense to me.  It conveniently allows decorating errors with an error code that can be used to map specific errors to http responses.  It doesn't blow up anything.