Error checking and defer

[Jsor]

I love defer, it helps you avoid problems by forgetting to unlock your mutex, or close your file, or whatever at a given return point. But there's one thing I don't like about it: it encourages you to ignore errors.

I like Go's error handling, I don't have a problem with the fact that it's "easy" to ignore errors, because you at least have to explicitly ignore them with _. So if it blows up, it's really your own fault. The problem with defer, though, is that in order to check an error you have to go out of your way.

Take the common example:

func DoStuff() (string, error) {

  myFile, err := os.Read("filename")

  defer myFile.Close()

  if err != nil {

     return "", errors.New("Couldn't open file: " + err.Error())

  }

  // Do stuff

  return someString, nil

}

Do you see anything wrong with this code? Well, you use defer so often you may not remember, but Close returns an error! And you're ignoring it! It hasn't been fed in a month and you don't even acknowledge its existence!

Now, if you do want to check the error, you can. I'm not arguing that you can't. The problem is, it's weird. You might think to write a handler function:

func ErrorHandler(errFunc func() error) {

  err := errFunc() //... uh... okay, now what?

}

Okay, so maybe that doesn't work

func ErrorHandler(errFunc func() error, err *error) {

  *err = errFunc()

}

Might be okay, now you can call

defer ErrorHandler(myFile.Close, &errorToReturn)

At least, I assume the address is still valid in a deferred function called after return.

Obviously you can use an anonymous function closure and assign the error to the named return value too, but then you have to rewrite, or copy/paste your error handling closure every time you defer an error throwing function. The other problem is that error handlers aren't necessarily portable, if the function takes different arguments you have to write a new handler for it.

I don't think this is good, it's one thing to allow a user to blatantly ignore an error with little effort. At least they're explicitly ignoring it, and it's really their own fault. It's quite another to have a commonly used language idiom that encourages programmers to ignore errors, and makes them expend extra effort just to make sure their program didn't do something weird. Fortunately, an error closing a File is pretty uncommon and usually won't cause anything too bad to happen, but checking errors is all about handling "shouldn't" and not just praying that what's usually okay still is. Not to mention that it's perfectly conceivable to have a deferrable function where the error it might return is not mostly superfluous. Unfortunately, I don't really have any ideas to fix it. I think disallowing defer to be used on functions with return values is going too far, and compiler warnings are out of the question.  I still feel like there needs to be some mechanism to not force programmers to expend extra effort to check an error, because that just leads to Bad Things™ happening. In a perfect world, programmers are going to check every error. In the real world a lot of programmers are going to go.

Thoughts?

[Jan Mercl]

On Tue, Oct 8, 2013 at 8:35 AM, Jsor <jrago...@gmail.com> wrote:
> Take the common example:
>
> func DoStuff() (string, error) {
> myFile, err := os.Read("filename")
> defer myFile.Close()
> if err != nil {
> return "", errors.New("Couldn't open file: " + err.Error())
> }
>
> // Do stuff
> return someString, nil
> }
>
> Do you see anything wrong with this code?

Yes. It schedules .Close of a potentially invalid myFile variable.
(Assuming s/Read/Open/ above.)

> Well, you use defer so often you
> may not remember, but Close returns an error! And you're ignoring it! It
> hasn't been fed in a month and you don't even acknowledge its existence!
>
> Now, if you do want to check the error, you can. I'm not arguing that you
> can't. The problem is, it's weird. You might think to write a handler
> function:

I think a direct approach is quite usable: http://play.golang.org/p/NIEzTx8sjP

-j

[Kamil Kisiel]

I think it's a fairly easy mistake to make if you are not careful. I typically use named return values when I need to capture an error from defer. Even then you need to be careful to not clobber any existing errors if your deferred call returns nil.

Recently errcheck (https://github.com/kisielk/errcheck) gained the ability to detect ignored errors from deferred calls like the Close example you gave. So if you run it on your code you should be able to see if you missed any.

[Nick Craig-Wood]

On 08/10/13 07:51, Jan Mercl wrote:
> I think a direct approach is quite usable: http://play.golang.org/p/NIEzTx8sjP

This is the solution I use which I got off this list a while back (can't
remember who posted it originally - sorry)

// checkClose is used to check the return from Close in a defer statement.
func checkClose(c io.Closer, err *error) {
cerr := c.Close()
if *err == nil {
*err = cerr
}
}

func DoStuff() (s string, err error) {

myFile, err := os.Read("filename")

defer checkClose(myFile, &err)
// ...
}

--
Nick Craig-Wood <ni...@craig-wood.com> -- http://www.craig-wood.com/nick

[Alexei Sholik]

Nick, your code has the same problem as the original one. You're ignoring the error returned by os.Read. myFile could be nil, in which case your checkClose() will panic.

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

--
Best regards
Alexei Sholik

[Nick Craig-Wood]

On 08/10/13 17:28, Alexei Sholik wrote:
> Nick, your code has the same problem as the original one. You're
> ignoring the error returned by os.Read. myFile could be nil, in which
> case your checkClose() will panic.

Sorry! It should be like this of course...

func DoStuff() (s string, err error) {
myFile, err := os.Read("filename")

if err != nil {
return
}

defer checkClose(myFile, &err)
// ...
}

> On Tue, Oct 8, 2013 at 6:56 PM, Nick Craig-Wood <ni...@craig-wood.com
> <mailto:ni...@craig-wood.com>> wrote:
>
> On 08/10/13 07:51, Jan Mercl wrote:
> > I think a direct approach is quite usable:
> http://play.golang.org/p/NIEzTx8sjP
>
> This is the solution I use which I got off this list a while back (can't
> remember who posted it originally - sorry)
>
> // checkClose is used to check the return from Close in a defer
> statement.
> func checkClose(c io.Closer, err *error) {
> cerr := c.Close()
> if *err == nil {
> *err = cerr
> }
> }
>
> func DoStuff() (s string, err error) {
> myFile, err := os.Read("filename")
> defer checkClose(myFile, &err)
> // ...
> }
>
> --

> Nick Craig-Wood <ni...@craig-wood.com <mailto:ni...@craig-wood.com>>

> -- http://www.craig-wood.com/nick
>
> --
> You received this message because you are subscribed to the Google
> Groups "golang-nuts" group.
> To unsubscribe from this group and stop receiving emails from it,
> send an email to golang-nuts...@googlegroups.com

> <mailto:golang-nuts%2Bunsu...@googlegroups.com>.

> For more options, visit https://groups.google.com/groups/opt_out.
>
>
>
>
> --
> Best regards
> Alexei Sholik

[Konstantin Khomoutov]

On Tue, 08 Oct 2013 17:40:47 +0100
Nick Craig-Wood <ni...@craig-wood.com> wrote:

[...]

> > This is the solution I use which I got off this list a while
> > back (can't remember who posted it originally - sorry)
> >
> > // checkClose is used to check the return from Close in a defer
> > statement.
> > func checkClose(c io.Closer, err *error) {
> > cerr := c.Close()
> > if *err == nil {
> > *err = cerr
> > }
> > }
> >
> > func DoStuff() (s string, err error) {
> > myFile, err := os.Read("filename")
> > defer checkClose(myFile, &err)
> > // ...
> > }
> >

> > Nick, your code has the same problem as the original one. You're
> > ignoring the error returned by os.Read. myFile could be nil, in
> > which case your checkClose() will panic.
>
> Sorry! It should be like this of course...
>
> func DoStuff() (s string, err error) {
> myFile, err := os.Read("filename")
> if err != nil {
> return
> }
> defer checkClose(myFile, &err)
> // ...
> }

This code still has a problem: it ignores the error which might happen
when the file is closed iff the function exits due to some error
condition.

In this particular example this might be sensible, as that possible
error happens when reading a file which closing we have deferred, and
so a problem with closing the file is supposedly irrelevant, but in
other cases it might be not: for instance, if an error generated in
the main code is not related to dealing with the resource which we
intend to finalize in our deferred code.
Another aspect is that freeing a resource might potentially be a
complex task in itself, for instance, we might use bufio layer between
the file on disk and our data source and call bufio.Flush () +
io.Close() in the deferred code, and flushing in the buffering layer
could error out due to no space left on device condition or something
like this which is serious (the data wasn't properly stored).
So I would panic in checkClose() if *err is not nil.
Or may be a more elaborate error handling is possible.

In any case just silencing an error detected while freeing a resource
when another one is pending to be returned to the callee is not a good
practice in the general case IMO.

[Kevin Gillette]

http://man7.org/linux/man-pages/man2/close.2.html#NOTES

tl;dr not checking the Close error after writing to a file can be, when you actually care, problematic. For example, if it were a log file that you were appending to, it may be irrelevant whether Close reveals an I/O error. If you were implementing a database or something equally critical, then checking for an error would be necessary. I can't imagine any situation in which it'd be useful to check the error value when closing a file that was opened read-only.

[Agis]

So why just not use `defer` when you care about errors?

What's your proposal?

[Nathan Kerr]

I prefer to accept that multiple errors can occur. https://pocketgophers.com/handling-errors-in-defer/