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: rows.Err() should probably return the last context.Err() #18961

Closed
kardianos opened this issue Feb 6, 2017 · 6 comments
Closed

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

kardianos opened this issue Feb 6, 2017 · 6 comments
Assignees
Milestone

Comments

@kardianos
Copy link
Contributor

@kardianos kardianos commented Feb 6, 2017

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.

err = rows.Scan(&r.age, &r.name)

Rows.Err() is then tested to ensure it returns no error at the end. This is probably a mistake as the rows was canceled.

err = rows.Err()

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.

@kardianos kardianos self-assigned this Feb 6, 2017
@bradfitz bradfitz added this to the Go1.8 milestone Feb 6, 2017
@bradfitz bradfitz added the NeedsFix label Feb 6, 2017
@rsc
Copy link
Contributor

@rsc rsc commented Feb 6, 2017

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?

@kardianos
Copy link
Contributor Author

@kardianos kardianos commented Feb 6, 2017

@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.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 6, 2017

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

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 7, 2017

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

@gopherbot gopherbot closed this in c026845 Feb 8, 2017
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 9, 2017

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

@rsc rsc reopened this Feb 9, 2017
@rsc
Copy link
Contributor

@rsc rsc commented Feb 9, 2017

CL 36614 for cherry-pick to Go 1.8 release branch.

gopherbot pushed a commit that referenced this issue Feb 9, 2017
… 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>
@rsc rsc closed this Feb 10, 2017
@golang golang locked and limited conversation to collaborators Feb 10, 2018
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
4 participants
You can’t perform that action at this time.