Documentation Request for deferring Close()

[Zamiell]

Hello, and thanks for the linter.

sqlclosecheck complains that rows.Close() should use defer. Can you explain why this is? For example, I don't see a difference between:

rows := db.Query("SELECT username FROM users")
for rows.Next() {
	// Do something
}
rows.Close()
return

and

rows := db.Query("SELECT username FROM users")
for rows.Next() {
	// Do something
}
defer rows.Close()
return

Putting defer here just seems confusing / a mistake / superfluous.

[ryanrolds]

Using a defer is preferred because if someone comes in later and puts an early return and doesn't make sure to also call Close then connection exhaustion is possible. This is a severe enough problem that is easy to miss that enforcing a defer is IMO a good course of action.

I agree that this linter should make sure that the defer is called immediately or very closely after a function returns the results/tx. I also agree that requiring a defer is stylistic. I will consider removing the defer requirement and instead check all branches call close.

[Zamiell]

Using a defer is preferred because if someone comes in later and puts an early return and doesn't make sure to also call Close then connection exhaustion is possible.

If that is the case, then I propose that the linter should also require that the "defer rows,Close()" is put immediately on the line after the "rows := ..." line.
(As opposed to just complaining that you have a "rows.Close()" somewhere. Because changing "rows.Close() to "defer rows.Close()" will shut up the linter but will not fix the bug.)

[ryanrolds]

I 100% agree and I've added these improvements to my list. The next step for me is to research how to enforce these rules as I'm fairly new to the ssa package.

[ammanley]

Not to crash your thread, but can I ask for a quick clarification?

Are we saying that we want to:

• enforce that a defer rows.Close() is happening immediately after the rows, err := db.Query('someSQLhere')?

And/Or:

• check all branches in the func to enforce that defer rows.Close() is happening somewhere, but just always before any (early) returns?

Would something along the lines of:

var rows *sql.Rows
defer rows.Close()

rows, err := db.Query('someSQLhere')
...function carries on

be suitable? Given we are defering the call to close before the chance for any early returns is introduced.

Edit: Thanks a lot for creating this, we identified a resource leak in our systems the other day and this linter helped us a lot in tracking down all the places. Much appreciate.

[ryanrolds]

It would not be immediately after db.Query(...) as the error should be checked first. If err isn't nil then rows should be nil. The plan is to have the defer right after the error check (when an error is returned with the cursor).

My fear with the last example is that if the error check returns and doesn't do something to make rows not nil then that logic may panic. But, maybe my in-head Go interpreter isn't right.

Glad I could help. :)

[freak12techno]

I have a kind of a false positive there:

for _, query := range queries {
	statement, err := db.Prepare(query)
	if _, err := statement.Exec(); err != nil {
		panic(err)
	}

	statement.Close()
}

If using defer, it won't work correctly as it's executed at the end of the function, as Goland suggests, and if not using it, the linter would complain.

[ryanrolds]

Defer in a loop can be tricky. A solution:

for _, query := range queries {
	func() {   
		statement, err := db.Prepare(query)
		if err != nil {
			panic(err)
		}
		defer statement.Close()

		if _, err := statement.Exec(); err != nil {
			panic(err)
		}
	}()
}