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
Comments
@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: 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:
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. |
The release notes (https://beta.golang.org/doc/go1.8#database_sql) do note 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.
Yes, perhaps that could be clarified.
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 |
CL https://golang.org/cl/34144 mentions this issue. |
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
)?What did you do?
https://play.golang.org/p/Y9jybN5zcQ
What did you expect to see?
What did you see instead?
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:
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.
The text was updated successfully, but these errors were encountered: