database/sql: rows.Err() should probably return the last context.Err() #18961
Comments
bradfitz
added
the
NeedsFix
|
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 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. |
|
CL https://golang.org/cl/36431 mentions this issue. |
|
CL https://golang.org/cl/36485 mentions this issue. |
|
CL https://golang.org/cl/36614 mentions this issue. |
|
CL 36614 for cherry-pick to Go 1.8 release branch. |
… if canceled Previously it was intended that Rows.Scan would return an error and Rows.Err would return nil. This was problematic because drivers could not differentiate between a normal Rows.Close or a context cancel close. The alternative is to require drivers to return a Scan to return an error if the driver is closed while there are still rows to be read. This is currently not how several drivers currently work and may be difficult to detect when there are additional rows. At the same time guard the the Rows.lasterr and prevent a close while a Rows operation is active. For the drivers that do not have Context methods, do not check for context cancelation after the operation, but before for any operation that may modify the database state. Fixes #18961 Change-Id: I49a25318ecd9f97a35d5b50540ecd850c01cfa5e Reviewed-on: https://go-review.googlesource.com/36485 Reviewed-by: Russ Cox <rsc@golang.org> Run-TryBot: Russ Cox <rsc@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-on: https://go-review.googlesource.com/36614 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Currently as of 91ad2a2 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.
go/src/database/sql/sql_test.go
Line 303 in 91ad2a2
Rows.Err() is then tested to ensure it returns no error at the end. This is probably a mistake as the rows was canceled.
go/src/database/sql/sql_test.go
Line 316 in 91ad2a2
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.
The text was updated successfully, but these errors were encountered: