[go] database/sql: Replace ctx.Done() with ctx.Err()
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded this change for review.
database/sql: Replace ctx.Done() with ctx.Err()
```
Prior to this commit, we used ctx.Done() to test for cancellation
without multiplexing over other channels like:
select {
default:
case <-ctx.Done():
// handle cancellation
}
context.Context.Done() lazily allocates the internal channel used and
there is some overhead to entering a select block.
This commit replaces lone ctx.Done() calls with ctx.Err() like below:
if err := ctx.Err(); err != nil {
// handle cancellation
}
This potentially avoids the channel allocation, avoids the overhead of
unnecessarily entering a select block, and saves a few lines and indent
levels. Still, it is probably more correct to use ctx.Err() when not
multiplexing over other channels.
benchstat output shows a measurable but negligible performance
improvement and no change to memory footprint.
name old time/op new time/op delta
ConcurrentDBExec-8 6.28ms ± 0% 6.27ms ± 0% ~ (p=1.000 n=1+1)
ConcurrentStmtQuery-8 5.61ms ± 0% 5.52ms ± 0% ~ (p=1.000 n=1+1)
ConcurrentStmtExec-8 4.29ms ± 0% 4.28ms ± 0% ~ (p=1.000 n=1+1)
ConcurrentTxQuery-8 5.55ms ± 0% 5.57ms ± 0% ~ (p=1.000 n=1+1)
ConcurrentTxExec-8 1.40ms ± 0% 1.39ms ± 0% ~ (p=1.000 n=1+1)
ConcurrentTxStmtQuery-8 1.63ms ± 0% 1.63ms ± 0% ~ (p=1.000 n=1+1)
ConcurrentTxStmtExec-8 454µs ± 0% 447µs ± 0% ~ (p=1.000 n=1+1)
ConcurrentRandom-8 4.66ms ± 0% 4.63ms ± 0% ~ (p=1.000 n=1+1)
ManyConcurrentQueries-8 10.4µs ± 0% 10.3µs ± 0% ~ (p=1.000 n=1+1)
name old alloc/op new alloc/op delta
ConcurrentDBExec-8 2.49MB ± 0% 2.50MB ± 0% ~ (p=1.000 n=1+1)
ConcurrentStmtQuery-8 2.25MB ± 0% 2.25MB ± 0% ~ (p=1.000 n=1+1)
ConcurrentStmtExec-8 1.71MB ± 0% 1.71MB ± 0% ~ (p=1.000 n=1+1)
ConcurrentTxQuery-8 2.37MB ± 0% 2.44MB ± 0% ~ (p=1.000 n=1+1)
ConcurrentTxExec-8 587kB ± 0% 587kB ± 0% ~ (all equal)
ConcurrentTxStmtQuery-8 745kB ± 0% 745kB ± 0% ~ (p=1.000 n=1+1)
ConcurrentTxStmtExec-8 108kB ± 0% 108kB ± 0% ~ (p=1.000 n=1+1)
ConcurrentRandom-8 1.63MB ± 0% 1.66MB ± 0% ~ (p=1.000 n=1+1)
ManyConcurrentQueries-8 3.76kB ± 0% 3.76kB ± 0% ~ (all equal)
name old allocs/op new allocs/op delta
ConcurrentDBExec-8 14.6k ± 0% 14.6k ± 0% ~ (all equal)
ConcurrentStmtQuery-8 17.1k ± 0% 17.1k ± 0% ~ (all equal)
ConcurrentStmtExec-8 3.17k ± 0% 3.17k ± 0% ~ (all equal)
ConcurrentTxQuery-8 19.6k ± 0% 19.6k ± 0% ~ (all equal)
ConcurrentTxExec-8 14.1k ± 0% 14.1k ± 0% ~ (all equal)
ConcurrentTxStmtQuery-8 19.2k ± 0% 19.2k ± 0% ~ (all equal)
ConcurrentTxStmtExec-8 2.17k ± 0% 2.17k ± 0% ~ (all equal)
ConcurrentRandom-8 13.9k ± 0% 13.9k ± 0% ~ (p=1.000 n=1+1)
ManyConcurrentQueries-8 17.0 ± 0% 17.0 ± 0% ~ (all equal)
```
This PR will be imported into Gerrit with the title and first
comment (this text) used to generate the subject and body of
the Gerrit change.
**Please ensure you adhere to every item in this list.**
More info can be found at https://github.com/golang/go/wiki/CommitMessage
+ The PR title is formatted as follows: `net/http: frob the quux before blarfing`
+ The package name goes before the colon
+ The part after the colon uses the verb tense + phrase that completes the blank in,
"This change modifies Go to ___________"
+ Lowercase verb after the colon
+ No trailing period
+ Keep the title as short as possible. ideally under 76 characters or shorter
+ No Markdown
+ The first PR comment (this one) is wrapped at 76 characters, unless it's
really needed (ASCII art, table, or long link)
+ If there is a corresponding issue, add either `Fixes #1234` or `Updates #1234`
(the latter if this is not a complete fix) to this comment
+ If referring to a repo other than `golang/go` you can use the
`owner/repo#issue_number` syntax: `Fixes golang/tools#1234`
+ We do not use Signed-off-by lines in Go. Please don't add them.
Our Gerrit server & GitHub bots enforce CLA compliance instead.
+ Delete these instructions once you have read and applied them
Change-Id: I1de2fdca200afd81e3bd1c9ade8ea02e19324ae5
GitHub-Last-Rev: 0adfad4fb70ae0b3467442b9f455c952291c447d
GitHub-Pull-Request: golang/go#43189
---
M src/database/sql/ctxutil.go
M src/database/sql/sql.go
2 files changed, 20 insertions(+), 30 deletions(-)
diff --git a/src/database/sql/ctxutil.go b/src/database/sql/ctxutil.go
index 4dbe6af..3ba740d 100644
--- a/src/database/sql/ctxutil.go
+++ b/src/database/sql/ctxutil.go
@@ -16,11 +16,9 @@
}
si, err := ci.Prepare(query)
if err == nil {
- select {
- default:
- case <-ctx.Done():
+ if err := ctx.Err(); err != nil {
si.Close()
- return nil, ctx.Err()
+ return nil, err
}
}
return si, err
@@ -35,11 +33,10 @@
return nil, err
}
- select {
- default:
- case <-ctx.Done():
- return nil, ctx.Err()
+ if err := ctx.Err(); err != nil {
+ return nil, err
}
+
return execer.Exec(query, dargs)
}
@@ -52,11 +49,10 @@
return nil, err
}
- select {
- default:
- case <-ctx.Done():
- return nil, ctx.Err()
+ if err := ctx.Err(); err != nil {
+ return nil, err
}
+
return queryer.Query(query, dargs)
}
@@ -69,11 +65,10 @@
return nil, err
}
- select {
- default:
- case <-ctx.Done():
- return nil, ctx.Err()
+ if err := ctx.Err(); err != nil {
+ return nil, err
}
+
return si.Exec(dargs)
}
@@ -86,11 +81,10 @@
return nil, err
}
- select {
- default:
- case <-ctx.Done():
- return nil, ctx.Err()
+ if err := ctx.Err(); err != nil {
+ return nil, err
}
+
return si.Query(dargs)
}
@@ -123,14 +117,14 @@
}
txi, err := ci.Begin()
+
if err == nil {
- select {
- default:
- case <-ctx.Done():
+ if err := ctx.Err(); err != nil {
txi.Rollback()
- return nil, ctx.Err()
+ return nil, err
}
}
+
return txi, err
}
diff --git a/src/database/sql/sql.go b/src/database/sql/sql.go
index 726aadb..158ceb4 100644
--- a/src/database/sql/sql.go
+++ b/src/database/sql/sql.go
@@ -1200,9 +1200,7 @@
return nil, errDBClosed
}
// Check if the context is expired.
- select {
- default:
- case <-ctx.Done():
+ if err := ctx.Err(); err != nil {
db.mu.Unlock()
return nil, ctx.Err()
}
@@ -2101,9 +2099,7 @@
var hookTxGrabConn func()
func (tx *Tx) grabConn(ctx context.Context) (*driverConn, releaseConn, error) {
- select {
- default:
- case <-ctx.Done():
+ if err := ctx.Err(); err != nil {
return nil, nil, ctx.Err()
}
To view, visit change 278112. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #2 to this change.
GitHub-Last-Rev: dbde1d8753965b60dd0c72af19eb909cb6ba8bef
GitHub-Pull-Request: golang/go#43189
---
M src/database/sql/ctxutil.go
M src/database/sql/sql.go
2 files changed, 20 insertions(+), 30 deletions(-)
To view, visit change 278112. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Daniel Theophanes, Brad Fitzpatrick.
4 comments:
Commit Message:
Patch Set #2, Line 7:
Replaces/Replace/replace/
No markdown syntax in commit message please.
Patch Set #2, Line 71:
This PR will be imported into Gerrit with the title and firstPlease delete all this boilerplate text from the commit message.
Patchset:
This will be for 1.17.
To view, visit change 278112. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Daniel Theophanes, Brad Fitzpatrick.
Gerrit Bot uploaded patch set #3 to this change.
database/sql: replace ctx.Done() with ctx.Err()
Prior to this commit, we used a select block with a default label and a read from
ctx.Done() to test for cancellation without blocking like:
Change-Id: I1de2fdca200afd81e3bd1c9ade8ea02e19324ae5
GitHub-Last-Rev: dbde1d8753965b60dd0c72af19eb909cb6ba8bef
GitHub-Pull-Request: golang/go#43189
---
M src/database/sql/ctxutil.go
M src/database/sql/sql.go
2 files changed, 20 insertions(+), 30 deletions(-)
To view, visit change 278112. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Daniel Theophanes, Brad Fitzpatrick.
Gerrit Bot uploaded patch set #4 to this change.
GitHub-Last-Rev: 4961ecd979a55923037bc1ad14d59ebb977e62c7
GitHub-Pull-Request: golang/go#43189
---
M src/database/sql/ctxutil.go
M src/database/sql/fakedb_test.go
M src/database/sql/sql.go
M src/database/sql/sql_test.go
M src/net/dial.go
M src/net/fd_unix.go
M src/net/fd_windows.go
M src/net/http/serve_test.go
M src/net/http/transport.go
M src/net/lookup.go
M src/os/exec/exec.go
11 files changed, 54 insertions(+), 87 deletions(-)
To view, visit change 278112. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Daniel Theophanes, Brad Fitzpatrick.
Gerrit Bot uploaded patch set #5 to this change.
database/sql,net,net/http,os/exec: Replace ctx.Done() with ctx.Err()
Prior to this commit, we performed non-blocking tests for cancellation
by receiving from ctx.Done() in a select block with a default label
like:
select {
case <-ctx.Done():
// handle cancellation
default:
}
Unless we're multiplexing over multiple channels, a better way to test
if a context is canceled is to test the result of ctx.Err()
This commit replaces all of the select{} based non-blocking cancellation
checks with a call to ctx.Err().
The src/context BenchmarkCheckCanceled benchmark demonstrates that
calling ctx.Err() is faster and, in general, it is probably clearer to
simply check ctx.Err() than using a select block.
Change-Id: I1de2fdca200afd81e3bd1c9ade8ea02e19324ae5
GitHub-Last-Rev: 4961ecd979a55923037bc1ad14d59ebb977e62c7
GitHub-Pull-Request: golang/go#43189
---
M src/database/sql/ctxutil.go
M src/database/sql/fakedb_test.go
M src/database/sql/sql.go
M src/database/sql/sql_test.go
M src/net/dial.go
M src/net/fd_unix.go
M src/net/fd_windows.go
M src/net/http/serve_test.go
M src/net/http/transport.go
M src/net/lookup.go
M src/os/exec/exec.go
11 files changed, 54 insertions(+), 87 deletions(-)
To view, visit change 278112. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Daniel Theophanes, Brad Fitzpatrick.
Gerrit Bot uploaded patch set #6 to this change.
database/sql,net,net/http,os/exec: Replace ctx.Done() with ctx.Err()
Prior to this commit, we performed non-blocking tests for cancellation
by receiving from ctx.Done() in a select block with a default label
like:
select {
case <-ctx.Done():
// handle cancellation
default:
}
Unless we're multiplexing over multiple channels, a better way to test
if a context is canceled is to test the result of ctx.Err()
This commit replaces all of the select{} based non-blocking cancellation
checks with a call to ctx.Err().
The src/context BenchmarkCheckCanceled benchmark demonstrates that
calling ctx.Err() is faster and, in general, it is probably clearer to
simply check ctx.Err() than using a select block.
Change-Id: I1de2fdca200afd81e3bd1c9ade8ea02e19324ae5
GitHub-Last-Rev: ba2fe38565545e7f9ee964b5cd99fea6d59e6a7b
GitHub-Pull-Request: golang/go#43189
---
M src/database/sql/ctxutil.go
M src/database/sql/fakedb_test.go
M src/database/sql/sql.go
M src/database/sql/sql_test.go
M src/net/dial.go
M src/net/fd_unix.go
M src/net/fd_windows.go
M src/net/http/serve_test.go
M src/net/http/transport.go
M src/net/lookup.go
M src/os/exec/exec.go
11 files changed, 59 insertions(+), 93 deletions(-)
To view, visit change 278112. To unsubscribe, or for help writing mail filters, visit settings.
Ayan George (Gerrit)
Attention is currently required from: Daniel Theophanes, Brad Fitzpatrick, Ian Lance Taylor.
Patch set 6:Code-Review +1
1 comment:
Patchset:
i think this is ready for review.
To view, visit change 278112. To unsubscribe, or for help writing mail filters, visit settings.
Emmanuel Odeke (Gerrit)
Attention is currently required from: Daniel Theophanes, Brad Fitzpatrick, Ian Lance Taylor.
Emmanuel Odeke removed a vote from this change.
To view, visit change 278112. To unsubscribe, or for help writing mail filters, visit settings.
Emmanuel Odeke (Gerrit)
Attention is currently required from: Daniel Theophanes, Brad Fitzpatrick, Ian Lance Taylor.
12 comments:
Commit Message:
Patch Set #6, Line 7:
database/sql,net,net/http,os/exec: Replace ctx.Done() with ctx.Err()all: replace context.Done() with context.Err() where necessary
This commit replaces all of the select{} based non-blocking cancellation
checks with a call to ctx.Err().
This change replaces, where necessary, the select{} based
non-blocking waits with a call to context.Err()Patch Set #6, Line 25:
The src/context BenchmarkCheckCanceled benchmarkcontext.BenchmarkCheckCanceled demonstrates
Patchset:
Thank you for this change Ayan, and great to see you contributing in 2021 too :-)
I've added some feedback to your change. Please rebase with the latest code from
the master branch, and please address the feedback below. When you are ready I shall
stamp it. Also, please don't +1 your own CLs :-)Thank you.
File src/database/sql/ctxutil.go:
if err == nil {
if err := ctx.Err(); err != nil {
txi.Rollback()
return nil, err
}
}
return txi, errPerhaps we could reverse conditional this and unnest it:
if err != nil {
return txi, err
}err = ctx.Err()
if err == nil {
return txi, nil
}// Otherwise rollback the transaction and discard it.
txi.Rollback()
return nil, err
File src/database/sql/sql.go:
Patch Set #6, Line 2149:
return tx.ctx.Err()Given that we already retrieved tx.ctx.Err() we can use it here.
return err
File src/database/sql/sql_test.go:
switch err := ctx.Err(); err {
case nil:
t.Fatalf("context err = nil; want context.Canceled")
default:
t.Fatalf("context err = %v; want context.Canceled", err)
case context.Canceled:
// expected context.Canceled
}We can simplify this to:
if err := ctx.Err(); err != context.Canceled {
t.Fatalf("context err = %v; want context.Canceled", err)
}Patch Set #6, Line 2964:
t.Fatal("timeout: failed to rollback query without closing rows:", err)t.Fatalf("timeout: failed to rollback query without closing rows: %v", err)
File src/net/http/serve_test.go:
select {
case <-ctx.Done():
t.Error("should not be Done in ServeHTTP")Please revert this, intentionally a select.
select {
case <-r.Context().Done():
t.Error("context unexpectedly canceled")
default:If we got this route and just replace singly, we've got to check for:
a) context.Canceled
b) err != nil, could be deadline exceeded and if so we need to figure out why
c) allow err == niland at that point it becomes a tedious check. Perhaps you can do this
if err := r.Context().Err(); err != nil {
t.Errorf("context unexpectedly expired: %v", err)
}Patch Set #6, Line 4809:
if ctx.Err() != nil {This is an incorrect translation, it should be that if ctx.Err() == nil,
that we have the failure. I'd say let's just revert and leave it as it was
otherwise if you retrieve an error we need to check for its value.
File src/net/http/transport.go:
Patch Set #6, Line 560:
if ctx.Err() != nil {Please use the value from ctx.Err():
if err := ctx.Err(); err != nil {
req.closeBody()
return nil, err
}
To view, visit change 278112. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Daniel Theophanes, Brad Fitzpatrick, Ian Lance Taylor.
Gerrit Bot uploaded patch set #7 to this change.
database/sql,net,net/http,os/exec: Replace ctx.Done() with ctx.Err()
Prior to this commit, we performed non-blocking tests for cancellation
by receiving from ctx.Done() in a select block with a default label
like:
select {
case <-ctx.Done():
// handle cancellation
default:
}
Unless we're multiplexing over multiple channels, a better way to test
if a context is canceled is to test the result of ctx.Err()
This commit replaces all of the select{} based non-blocking cancellation
checks with a call to ctx.Err().
The src/context BenchmarkCheckCanceled benchmark demonstrates that
calling ctx.Err() is faster and, in general, it is probably clearer to
simply check ctx.Err() than using a select block.
Change-Id: I1de2fdca200afd81e3bd1c9ade8ea02e19324ae5
GitHub-Last-Rev: 701e35f7abaa031ef66c7b0ba6a138c85a54794f
GitHub-Pull-Request: golang/go#43189
---
M src/database/sql/ctxutil.go
M src/database/sql/fakedb_test.go
M src/database/sql/sql.go
M src/database/sql/sql_test.go
M src/net/dial.go
M src/net/fd_unix.go
M src/net/fd_windows.go
M src/net/http/serve_test.go
M src/net/http/transport.go
M src/net/lookup.go
M src/os/exec/exec.go
11 files changed, 56 insertions(+), 91 deletions(-)
To view, visit change 278112. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Daniel Theophanes, Brad Fitzpatrick, Ian Lance Taylor.
Gerrit Bot uploaded patch set #8 to this change.
database/sql,net,net/http,os/exec: Replace ctx.Done() with ctx.Err()
Prior to this commit, we performed non-blocking tests for cancellation
by receiving from ctx.Done() in a select block with a default label
like:
select {
case <-ctx.Done():
// handle cancellation
default:
}
Unless we're multiplexing over multiple channels, a better way to test
if a context is canceled is to test the result of ctx.Err()
This commit replaces all of the select{} based non-blocking cancellation
checks with a call to ctx.Err().
The src/context BenchmarkCheckCanceled benchmark demonstrates that
calling ctx.Err() is faster and, in general, it is probably clearer to
simply check ctx.Err() than using a select block.
Change-Id: I1de2fdca200afd81e3bd1c9ade8ea02e19324ae5
GitHub-Last-Rev: d6f2b3e38ccb48e3649ece72556522b1e26715e4
GitHub-Pull-Request: golang/go#43189
---
M src/database/sql/ctxutil.go
M src/database/sql/fakedb_test.go
M src/database/sql/sql.go
M src/database/sql/sql_test.go
M src/net/dial.go
M src/net/fd_unix.go
M src/net/fd_windows.go
M src/net/http/serve_test.go
M src/net/http/transport.go
M src/net/lookup.go
M src/os/exec/exec.go
11 files changed, 56 insertions(+), 91 deletions(-)
To view, visit change 278112. To unsubscribe, or for help writing mail filters, visit settings.