Go: database/sql: rows.Err() should probably return the last context.Err()

Created on 6 Feb 2017 · 6Comments · Source: golang/go

Currently as of 91ad2a219445d6df3ddb796d0f44190da24f429d if the context is canceled during Rows read, rows.Err will not return an error.

Currently the QueryContext test expects Scan to return an error if the row set is closed before it is done. This is easy to detect and perform on the dummy driver. It is less easy to do on a real wire driver.
https://github.com/golang/go/blob/91ad2a219445d6df3ddb796d0f44190da24f429d/src/database/sql/sql_test.go#L303

Rows.Err() is then tested to ensure it returns no error at the end. This is probably a mistake as the rows was canceled.
https://github.com/golang/go/blob/91ad2a219445d6df3ddb796d0f44190da24f429d/src/database/sql/sql_test.go#L316

It would probably be better to mark rows.lasterr = ctx.Err() on ctx cancel.

This was reported by @jackc with https://github.com/jackc/context-rows-cancel/blob/master/main.go .

We could ask drivers to alter their behavior to return an error when close is called, rather then stop returning rows. But that might be hard to get right; it is a change from what exists today. Because we have the actual context error, I recommend we make Rows.Err return the context cancel error.

FrozenDueToAge NeedsFix

Source

kardianos

Most helpful comment

@rsc this is a result from new code in go1.8. I would like to fix it in go1.8 because it won't be possible to fix it in go1.9 as it would start returning errors where none were returned before in valid code.

This code is working as intended for go1.8, but as @jackc pointed out to me, it doesn't work well with existing drivers. I'll send a CL, you can see if it is worth it.

I'm sorry for the series of late issues in database/sql this cycle. I'm taking steps to ensure this doesn't happen in the next one.

kardianos on 6 Feb 2017

👍5

All 6 comments

This doesn't sound urgent enough for Go 1.8. Is this code new in Go 1.8, or is the bug new since Go 1.7?

rsc on 6 Feb 2017

This code is working as intended for go1.8, but as @jackc pointed out to me, it doesn't work well with existing drivers. I'll send a CL, you can see if it is worth it.

I'm sorry for the series of late issues in database/sql this cycle. I'm taking steps to ensure this doesn't happen in the next one.

kardianos on 6 Feb 2017

👍5

CL https://golang.org/cl/36431 mentions this issue.