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

database/sql: at least one example should handle rows.Close() error #24329

Closed
kevinburke opened this issue Mar 9, 2018 · 9 comments
Closed

database/sql: at least one example should handle rows.Close() error #24329

kevinburke opened this issue Mar 9, 2018 · 9 comments

Comments

@kevinburke
Copy link
Contributor

@kevinburke kevinburke commented Mar 9, 2018

Presumably, sql.Rows.Close() can return an error, and should be checked in code where correctness matters.

Adapting the examples to check this error isn't trivial, here's a mistake I just made in production code, for example:

func doQuery() error {
	age := 27
	rows, err := db.Query("SELECT name FROM users WHERE age=?", age)
	if err != nil {
		return err
	}
	for rows.Next() {
		var name string
		if err := rows.Scan(&name); err != nil {
			return err
		}
		fmt.Printf("%s is %d\n", name, age)
	}
	if err := rows.Close(); err != nil {
		return err
	}
	if err := rows.Err(); err != nil {
		return err
	}
}

The error is if Scan() fails, we never call rows.Close() and hold a connection open forever.

Similarly, the docs should give guidance on whether you should check Err or Close first. (Maybe it doesn't matter).

@dgryski
Copy link
Contributor

@dgryski dgryski commented Mar 9, 2018

When this is clarified, the tutorial at https://github.com/VividCortex/go-database-sql-tutorial should probably also be updated. Although that seems mostly unmaintained at the moment.

@andybons andybons added this to the Unplanned milestone Mar 10, 2018
@agnivade
Copy link
Contributor

@agnivade agnivade commented Mar 10, 2018

According to docs -

Close is idempotent and does not affect the result of Err.

So I guess it doesn't matter.

How about we change the example to -

age := 27
rows, err := db.Query("SELECT name FROM users WHERE age=?", age)
if err != nil {
        log.Fatal(err)
}
defer func(){
  err := rows.Close()
  if err != nil {
    log.Fatal(err)
  }
}()
for rows.Next() {
        var name string
        if err := rows.Scan(&name); err != nil {
                log.Fatal(err)
        }
        fmt.Printf("%s is %d\n", name, age)
}
if err := rows.Err(); err != nil {
        log.Fatal(err)
}
@kardianos
Copy link
Contributor

@kardianos kardianos commented Mar 10, 2018

Fair points. No log fatal in examples. I think checking the scan error and rows.err is redundant. Could just return result of close. Also it the ctx is canceled rows will close.

@romainmenke
Copy link

@romainmenke romainmenke commented Mar 12, 2018

Better even to use the double close pattern :

age := 27
rows, err := db.Query("SELECT name FROM users WHERE age=?", age)
if err != nil {
        log.Fatal(err)
}
defer rows.Close()

for rows.Next() {
        var name string
        if err := rows.Scan(&name); err != nil {
                log.Fatal(err)
        }
        fmt.Printf("%s is %d\n", name, age)
}

if err := rows.Err(); err != nil {
        log.Fatal(err)
}

if err := rows.Close(); err != nil {
        log.Fatal(err)
}
@bcmills
Copy link
Member

@bcmills bcmills commented Mar 14, 2018

@romainmenke I'm curious, why do you say that the double-close pattern is better?

Not every io.Closer is idempotent, so that pattern is dangerous in general (but safe for sql.Rows specifically).

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 14, 2018

the docs should give guidance on whether you should check Err or Close first. (Maybe it doesn't matter).

Per http://go/godoc/database/sql/#Rows.Close,
“If Next is called and returns false and there are no further result sets, the Rows are closed automatically and it will suffice to check the result of Err.”

That seems to imply that you should call Err if Next returns false, and Close otherwise: in the common case, you need only one or the other. (And per #20803 (comment), you can safely ignore the error from Close: by the time you call it, either you're returning an error or you've successfully read all of the data you intended to read.)

That suggests a variation on @romainmenke's suggestion, omitting the second Close.

func doQuery(age int) error {
	rows, err := db.Query("SELECT name FROM users WHERE age=?", age)
	if err != nil {
		return err
	}
	defer rows.Close()

	for rows.Next() {
		var name string
		if err := rows.Scan(&name); err != nil {
			return err
		}
		fmt.Printf("%s is %d\n", name, age)
	}
	return rows.Err()
}
@agnivade
Copy link
Contributor

@agnivade agnivade commented Aug 28, 2018

@bcmills - I am not very clear from your linked comment about why we can ignore the error from .Close(). As I understand, if Next() returns false, it is enough to just check for .Err(), but if we fail inside the loop, then we need to .Close() manually (and also check for the error ?)

If we don't need to check for error from .Close(), then I don't see anything else to be done in the issue. Because the actual example in the docs does not return anything. So we need to check the error of rows.Err(); which is already being done in the example.

@bcmills
Copy link
Member

@bcmills bcmills commented Sep 6, 2018

@agnivade, I think you're correct. The variation I suggested is almost exactly how ExampleRows is already written today, right down to the defer rows.Close(). 🙂

I agree that there doesn't seem to be anything else to be done. Other folks on the issue, please let us know if we've missed something.

@bcmills bcmills closed this Sep 6, 2018
@suederade
Copy link

@suederade suederade commented Jul 15, 2019

Wouldn't @agnivade be the most appropriate at least for rows.Close() because it actually returns an error?

@golang golang locked and limited conversation to collaborators Jul 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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