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

proposal: cmd/vet: detect defer rows.Close() #34544

Closed
zachgersh opened this issue Sep 25, 2019 · 10 comments
Closed

proposal: cmd/vet: detect defer rows.Close() #34544

zachgersh opened this issue Sep 25, 2019 · 10 comments
Labels
Projects
Milestone

Comments

@zachgersh
Copy link

@zachgersh zachgersh commented Sep 25, 2019

I'd like to suggest, based on the prior art of #17780, that we also add a check to make sure that rows.Close() is always deferred for any SQL call that returns rows). I struggle to think of a false positive especially due to the fact that rows.Close() can be safely deferred in every case.

Consider this code:

        _, err := db.Exec(`INSERT into (age) VALUES (30)`)
        if err != nil {
                return err
        }

Let's say I've done this in some test setup, I'd like to clean up the DB for another set of tests, I am now unable to truncate the database due to the fact that I have left this connection open. Production code won't run into this truncation issue (I hope) but it could potentially put pressure on the database due to the number of open connections instead.

A False positive could potentially look like this:

        rows, err := db.QueryContext(ctx, "SELECT name FROM users WHERE age=?", 10)
	if err != nil {
		log.Fatal(err)
	}

	for rows.Next() {
	}

My for loop will have consumed all the rows and I therefore get a close call automatically. We still show a deferred close here for users just in case they decide to break out of their for loop early thus turning this back into a problem.

@gopherbot gopherbot added this to the Proposal milestone Sep 25, 2019
@gopherbot gopherbot added the Proposal label Sep 25, 2019
@rsc rsc added this to Incoming in Proposals Dec 4, 2019
@rsc
Copy link
Contributor

@rsc rsc commented Apr 8, 2020

/cc @robpike @kardianos but this does seem OK given #17780.
It would be nice to have some numbers about how often this happens in the wild.
(See cmd/vet/README.)

@rsc rsc moved this from Incoming to Active in Proposals Apr 8, 2020
@nemith
Copy link
Contributor

@nemith nemith commented Apr 8, 2020

I have some people on my team at work new to Go and we had two outages due to rows.Close() missing. What makes this worse is with the mysql driver this results in leaked goroutines and fails in random ways and it's not very clear from the failures or the goroutine leak that the code was missing rows.Close().

Now some of this could be fixed by some changes in the way packets and errors are handled in the mysql driver but having a clear vet would make it easier to discover the problem. We ended up just manually scanning the changed code looking for the missing calls which is a lot more error prone.

@robpike
Copy link
Contributor

@robpike robpike commented Apr 8, 2020

Seems OK to me based on the story here, but I am not a user of that package.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Apr 8, 2020

Any helper function that wraps Query and returns Rows would probably be a false positive. I don't know how common those are.

QueryContext will close Rows when ctx is canceled.

I typically buffer the entire result set and close the Query by default in part to avoid this type of issue, so it can be a problem.

In short, it would probably be okay, but I'm guessing for ~1% of users it will display a false positive, though that may be high. (defering rows.Close wouldn't work in a helper function that calls Query and returns Rows.)

@zachgersh
Copy link
Author

@zachgersh zachgersh commented Apr 9, 2020

(defering rows.Close wouldn't work in a helper function that calls Query and returns Rows.)

How does this work now if one returns a resp from a helper function in the eyes of vet? Seems like pretty much the same issue as this one to me.

@rsc
Copy link
Contributor

@rsc rsc commented Apr 15, 2020

Vet checks aim to have zero false positives, or else false positives with an easy rewrite to silence the error. I don't see a rewrite here for wrappers other than something like if false { rows.Close() }. That would be a problem.

At the least we need more data here. But we also need a story for why there won't be unfixable (or only fixable with ugly code) false positives. Maybe the check would have to look for rows being returned and understand that close need not be called. But then that would apply to rows being put in data structures too. Or sent on channels. It's unclear where it ends.

Maybe one could define a clear "rows does not escape" signal, like only methods on rows are invoked and it is never copied anywhere (not into other functions, channels, data structures, etc). We'd then need to check and see if that catches useful bugs.

(I guess the http.Response Body doesn't get wrapped or copied around as much?)

@kardianos
Copy link
Contributor

@kardianos kardianos commented Apr 15, 2020

@rsc I agree this needs more information. I'll see what I can dig up this week for data.

I picked a random "used by" on pkg.go.dev and the first one I got was this:
https://github.com/readystock/goqu/blob/v5.0.0/database.go#L216

func (me *Database) Query(query string, args ...interface{}) (*sql.Rows, error) {
	me.Trace("QUERY", query, args...)
	return me.Db.Query(query, args...)
}

Which I think would fail the vet check proposed.


One possibility is to add this check ONLY if rows.Scan is also called in the same method. I think this should be safe 100% of the time. But I should probably do a corpus analysis before making that judgement.

@rsc
Copy link
Contributor

@rsc rsc commented Apr 22, 2020

We can wait a little longer for data but it's really starting to sound like this is a no-go. The false positive with no clear workaround really sinks this from a vet perspective. We'd need not just data but also a design without that problem.

@rsc
Copy link
Contributor

@rsc rsc commented Apr 29, 2020

The discussion above has identified that there are common patterns that will cause false positives in this check that will be quite difficult (or at least ugly and non-obvious) to work around.

Based on that, this seems like a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals Apr 29, 2020
@rsc
Copy link
Contributor

@rsc rsc commented May 6, 2020

No change in consensus, so declined.

@rsc rsc moved this from Likely Decline to Declined in Proposals May 6, 2020
@rsc rsc closed this May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.