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

Unnecesary rows.Close() after Query #19

Open
tomachine opened this issue Feb 21, 2023 · 6 comments
Open

Unnecesary rows.Close() after Query #19

tomachine opened this issue Feb 21, 2023 · 6 comments
Labels
enhancement New feature or request

Comments

@tomachine
Copy link

tomachine commented Feb 21, 2023

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

image (2)
image (3)

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.

@ryanrolds
Copy link
Owner

Thank you for filing the issue. I will take a look at it.

@jtorvald
Copy link

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.

@ryanrolds
Copy link
Owner

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.

@ryanrolds ryanrolds added the enhancement New feature or request label Aug 20, 2023
@jtorvald
Copy link

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

@ryanrolds
Copy link
Owner

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.

@ryanrolds
Copy link
Owner

ryanrolds commented Aug 27, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants