Gidelines or best practices on error on error handling?

[josvazg]

Is there some idiomatic or well known way to deal with extra errors when you are already recovering from or reporting an error condition?

The typical case is, you hit an error and need to release resources before returning it. But the resources themselves may fail when closing them.

One (frequent) way seems to be to defer the freeing/closing so the resource will be freed on exit of the call no matter what. But that way you could be swallowing resource closing errors, which could lead to long debugging sessions in production.

Another way is to have an adhoc close & report wrapper function to use when deferring resource closing call that may fail. The wrapper can log the fact that a closing error happened while reporting some other error, then choose to return one or the other up the stack. But that can make the code ugly and difficult to read. Or is there a way to wrap 2 errors at once somehow?

Are there better ways to handle this situations?

Thanks,

Jose

[Uli Kunitz]

You need to think about how important it is for the user of your function or method to know about the Close error. Usually when a function closes a resource it also acquires it, so your caller cannot do anything to fix the closure. So I report only the first error.

The question that is more difficult to answer is, should errors of Close functions be reported, if there is no error in the function. The typical defer f.Close() doesn't do it. If your code is mission critical you should do it, but it will require something like:

defer func() {

    if cerr := f.Close(); cerr != nil && err == nil {

        err = cerr

    }

}()

The variable err must be of course a named return value. I would recommend to do it, if this is mission-critical code. Another approach might be the following,

f , err := os.Open(....)

if err {

    return err

}

defer f.Close()

// work with f

if err := f.Close(); err != nil {

    return err

}

This requires that you can call Close twice and the second Call is essentially a no-op potentially returning an error. You must also ensure that all successful path end at the f.Close call.

[Amnon]

I really do not like code like
defer func() {

    if cerr := f.Close(); cerr != nil && err == nil {

        err = cerr

    }

}()

My problem with this code is:
1) It is ugly
2) It is too clever
3) It is not clear what the code actually does
4) It is a trap for maintainers of the code

This code _seems_ to pass any error returned by f.Close() to the enclosing function. 
But it only works if the function has a named return parameter. If somebody later 
changes the function to return unnamed values, this defer will silently fail.

This happens all the time out in the wild.

(Check out
https://github.com/blevesearch/bleve/blob/369ad22b9fefdadb42b9367193ef7a73b9e5f4ff/search/searcher/search_regexp.go#L54
for instance).

So my advice would be 
1) Keep the code simple
2) Don't use defer in places where it leads to convoluted code.
3) It is often OK ignore the error returned by f.Close()
e.g. if f is a Reader, a bytes.Buffer, or a http.ResponseWriter.

- amnon

[Jan Mercl]

On Sun, Jan 31, 2021 at 8:04 AM Amnon <amn...@gmail.com> wrote:

> I really do not like code like
> defer func() {
> if cerr := f.Close(); cerr != nil && err == nil {
> err = cerr
> }
> }()

..

> If somebody later
> changes the function to return unnamed values, this defer will silently fail.

The deferred function literal refers to a named return variable.
Changing it to an unnamed one will cause the program to no longer
compile.

[Amnon]

The deferred function literal refers to a named return variable.
Changing it to an unnamed one will cause the program to no longer
compile.

Interesting....

https://play.golang.org/p/jnRn4Bv98xS

See the example on the Go Playground.

Jan, someone can explain to me why it will no longer compile?

- amnon

[Jan Mercl]

On Sun, Jan 31, 2021 at 5:24 PM Amnon <amn...@gmail.com> wrote:

> https://play.golang.org/p/jnRn4Bv98xS
>
> See the example on the Go Playground.
>
> Jan, someone can explain to me why it will no longer compile?

That's not an example of what I wrote: "Changing it to an unnamed one

will cause the program to no longer compile."

Your example is "making the return variable unnamed and declaring a
new, different variable of the same name in the same scope".

[Amnon]

OK.

If you take the 
https://play.golang.org/p/L8z2Q_F_fi3

and change the line 18 
from 
func foo() (err error) {

to
func foo() error {

The code will continue to compile.
But it will stop working (i.e. the error from f.Close will not be returned by foo).

Unfortunately this is not a contrived example. 
The earlier link I posted 
https://github.com/blevesearch/bleve/blob/369ad22b9fefdadb42b9367193ef7a73b9e5f4ff/search/searcher/search_regexp.go#L54
is bleve, a search engine with 7.4K stars on Github which is widely used in production.

The moral of the story is: don't write convoluted code which is hard to understand,
and which is prone to silent breakages.

[Jan Mercl]

On Sun, Jan 31, 2021 at 6:01 PM Amnon <amn...@gmail.com> wrote:
> If you take the
> https://play.golang.org/p/L8z2Q_F_fi3
>
> and change the line 18
> from
> func foo() (err error) {
>
> to
> func foo() error {
>
> The code will continue to compile.

That's again the case of "making the return variable unnamed and declaring a

new, different variable of the same name in the same scope".

The fact that the above is in your example achieved simply by deleting
the text of the name of the named return variable is not relevant to
what I wrote. That's about the semantics. IOW, redirecting the
resolution of the named return variable to a different, non return
variable, which in your example `err` becomes.

I hope it's clear now. Howgh.