ryanrolds
Unnecesary rows.Close() after Query
When using db.Query(query) the Rows/Stmt was not closed error appears, even though I'm checking the Err() method. The documentation says that Close() is called automatically and checking of Err() is sufficient.
Expected: No linter error when not calling Close() excplicitly and checking using Err() method. Actual: Not closed linter error
rows, err := d.db.Query(query)
if err != nil {
return nil, err
}
for rows.Next() {
//do something
}
if rows.Err() != nil {
return nil, err
}
This code results in no close error.
I don't know about this. Yes the docs say it does close rows when finished iterating but I had memory leaks because of not closing rows in the past. Not sure what might have caused that. Just thinking, what if something panics during iterating and you don't finish to the last rows in the set, would it then never close the result and leak?
I think it's better to always explicitly close in defer just to make sure you don't miss any. .Err() does not seem to do anything with the close except checking for errors. So would you still need to call rows.Close if there is an error?
The example in the docs also does a defer rows.Close even if it checks errors.
@ryanrolds thanks for this vettool, I have been using sqlrows before but it seems no longer be maintained.
I understand the point you're making. This linter enforces the belief that Close should always be called for safety. It's easy to refactor code and be unaware that a close needs to be added.
Ignoring cases that don't need Close called is possible and if enough people want it added, I will look into it. Not going to close it, anyone wanting this please speak up.
I believe that if .Err() closes it, it's the same as calling Close. In that case you don't want a false positive, otherwise your start to avoid the linter if this is the way you normally write code. If someone changes the code, the linter will warn for the lack of closing in case the .Err() is removed. Just my 2ct
I will look at implementing a more complex check that confirms things are handled appropriately. I don't have an ETA as this is pushing up against my current linter expertise.
I'm starting work on a refactored analyzer supporting your example. The trickiest thing will be analyzing the Next() loop and ensuring it doesn't return from inside of the loop. Or not exit the loop early.