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: Context cancellation doesn't #18168

Closed
j7b opened this issue Dec 2, 2016 · 3 comments
Closed

database/sql: Context cancellation doesn't #18168

j7b opened this issue Dec 2, 2016 · 3 comments

Comments

@j7b
Copy link

@j7b j7b commented Dec 2, 2016

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.8beta1 linux/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/root"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0"
CXX="g++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

https://play.golang.org/p/Y9jybN5zcQ

What did you expect to see?

Hello bub
delete error  context deadline exceeded
Hello bub

What did you see instead?

Hello bub
delete error  context deadline exceeded
No bub!

The output demonstrates the expectation when using a Context type isn't satisfied (cancellation doesn't happen and resources are not freed) and actual driver method output is discarded (discarding error types, even if nil, returned by the driver doesn't seem a reasonable design).

As implemented, at the very least I'd expect the methods with a Context parameter to panic if the driver doesn't implement what must be an optional interface.

On a related note:

    // Deprecated: Drivers should implement ConnBeginContext instead (or additionally).
    Begin() (Tx, error)

Is there a technical reason for deprecating Begin() for an interface used by a package described as "a generic interface around SQL (or SQL-like) databases," not "a generic interface around SQL (or SQL-like) databases with a practical cancellation facility?" The "instead" doesn't make sense, you can't not implement Begin() and fulfill driver.Conn. The "additionally" is also a little bizarre, if it's indeed deprecated why implement it? Why would Begin() be deprecated and Prepare() not? Why would ConnBeginContext and ConnPrepareContext be separate interfaces, shouldn't an implementation that's faking one or the other be required to fake both?

It seems there's a need to review this project's proposal procedures, code review process, compatibility guidelines, and general regard for the value of community implementations of standard library interfaces.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Dec 2, 2016

@j7b Hi Jake. I'm not sure how to respond to the last two paragraphs.

Let me address the cancelation expectation. The context can help in multiple locations: getting a free connection from the pool (there were multiple bugs open where a depleted pool could block forever), canceling a query during execution as you are testing above if the driver supports it (Please see https://docs.google.com/spreadsheets/d/1y7AzkFNPeTBado0xJJipB5MpWlcqylQg410Q5wHz2Ew/edit#gid=0 for implementation status, right now the driver you used doesn't support in-flight cancelation), it also help ensure connections are returned to the pool when you are done with it.

Yes, you still need a Begin() function, but if you implement BeginContext, your Begin function can be:
func (Conn) Begin() (Tx, error) {panic("use BeginContext")} as it won't ever be called. In the future where go1.8 to be the minimum I expect all drivers to be implemented as such, where Begin is just a stub. So in that sense, yes, it is deprecated.

These issues and CLs have been active for many months. You can review the history of them if you'd like. I think you even commented on some of the issues.

I don't understand what you mean here at all:

It seems there's a need to review this project's proposal procedures, code review process, compatibility guidelines, and general regard for the value of community implementations of standard library interfaces.

Does it seem like core go members are disregarding community involvement? I'm unsure of your intent.


As far as I can tell, for the first issue you raised this is working as expected. I'll keep this open if you'd like to clarify some of your secondary comments.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 2, 2016

The release notes (https://beta.golang.org/doc/go1.8#database_sql) do note that:

enables canceling in-progress queries should the driver support that

But https://beta.golang.org/pkg/database/sql/ lacks that documentation. Thanks for the report. We did try to clarify everywhere in the package docs where features weren't available if the underlying driver doesn't support it.

// Deprecated: Drivers should implement ConnBeginContext instead (or additionally).

Yes, perhaps that could be clarified.

It seems there's a need to review this project's proposal procedures, code review process, compatibility guidelines, and general regard for the value of community implementations of standard library interfaces.

I'm not sure whether you intended for that to come across as passive aggressive. If you'd like to get involved in code reviews (https://golang.org/doc/contribute.html), or the proposal process (https://github.com/golang/proposal), you're certainly welcome. Even this process now with betas is part of a process (https://github.com/golang/go/wiki/Go-Release-Cycle). And we have a final-final API review every 6 months (e.g. https://groups.google.com/forum/#!topic/golang-dev/t5rvbW6OK9U, which generated feedback, as it tends to) before we lock in the API for later (https://golang.org/doc/go1compat). If you find any of our process flawed, please file a separate bug, and let's keep this bug about database/sql.

In any case, we'll work on the docs.

Thanks for the report.

/cc @kardianos

@bradfitz bradfitz added this to the Go1.8 milestone Dec 2, 2016
@bradfitz bradfitz self-assigned this Dec 2, 2016
@quentinmit quentinmit added the NeedsFix label Dec 6, 2016
@gopherbot
Copy link

@gopherbot gopherbot commented Dec 8, 2016

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

@gopherbot gopherbot closed this in e12ce1e Dec 9, 2016
@golang golang locked and limited conversation to collaborators Dec 9, 2017
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
5 participants
You can’t perform that action at this time.