Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ignore defer calls #55

Closed
dmitris opened this issue Dec 19, 2014 · 7 comments
Closed

ignore defer calls #55

dmitris opened this issue Dec 19, 2014 · 7 comments

Comments

@dmitris
Copy link

@dmitris dmitris commented Dec 19, 2014

currently errcheck finds a lot of calls like defer r.Body.Close() or defer db.Close(). Would it be possible to ignore them to make the tool's output more meaningful and get the real problems to better stand out? Thanks!

@dominikh

This comment has been minimized.

Copy link
Collaborator

@dominikh dominikh commented Dec 19, 2014

Odds are that most unchecked deferred Close's are exactly the kind of things errcheck should report.

Consider the following piece of code:

f, err := os.Create("foo")
if err != nil {
...
}
defer f.Close()
f.Write([]byte("something"))

This is clearly a Close that can return an error that should be checked. Same goes for closing database transactions.

If you want to hide specific Close's, that are known to always return nil, consider using the -ignore flag.

@dmitris

This comment has been minimized.

Copy link
Author

@dmitris dmitris commented Dec 29, 2014

I would argue that in your snippet it is important to check and handle the error from f.Write - and maybe less important to check the result of f.Close() as it is anyway executed at the end of the function call and the variable f will get deallocated anyway.

Consider also the documentation and example from http://golang.org/pkg/net/http/ - it does not use error checking for closing the response body:

resp, err := http.Get("http://example.com/")
if err != nil {
    // handle error
}
defer resp.Body.Close()
body, err := ioutil.ReadAll(resp.Body) 

Of course the defaults can be set in different ways and there are tradeoffs involved - personally, I find its too bad that the default output gets polluted with many of "defer r.Body.Close()".

For reference, you can check for errors as described in the answer to http://stackoverflow.com/questions/19934641/golang-returning-from-defer

@dominikh

This comment has been minimized.

Copy link
Collaborator

@dominikh dominikh commented Dec 29, 2014

Closing the response body of an HTTP request you only read from cannot yield a meaningful error, no, and closing here is done to return the connection to the pool.

My example, however, wasn't using HTTP, but files. If you write to a file, and that write didn't return an error, closing the file can still return an error indicating that the write itself failed. Why? caching by the OS, for one. So if you don't check that Close's error, you're writing buggy code that can potentially lose data.

Also I'm not sure what that question on SO has to do with this. We're not panicing, nor recovering panics. And if you wanted to check the error return of Close in a defer, you'd do so in a way that errcheck wouldn't complain about, such as

func foo() (err error) {
  f, _ := os.Create("foo")
  defer func() {
    if err != nil {
      err = f.Close()
    }
  }()
  ...
}

(left out error handling of os.Create for brevity, not correctness.)

@dmitris

This comment has been minimized.

Copy link
Author

@dmitris dmitris commented Jan 22, 2015

ok, let's close this

@dmitris dmitris closed this Jan 22, 2015
@dominikh dominikh mentioned this issue Aug 11, 2015
@nevet

This comment has been minimized.

Copy link

@nevet nevet commented Oct 26, 2015

@dominikh could you explain why you choose to do f.Close() when err is already not nil?

@dominikh

This comment has been minimized.

Copy link
Collaborator

@dominikh dominikh commented Oct 27, 2015

@nevet That code of mine looks wrong. It should always call f.Close. Whether it should overwrite an already non-nil error depends on the given requirements.

@afeld

This comment has been minimized.

Copy link

@afeld afeld commented Jan 2, 2018

Here's what I ended up doing:

func foo() error {
  f, err := os.Create("foo")
  if err != nil {
    return err
  }
  
  defer func() {
    err = f.Close()
    if err != nil {
      log.Fatalln(err)
    }
  }()
  // ...
}
tongson added a commit to tongson/rr that referenced this issue Sep 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.