Add support for context.Context #608

shogo82148 · 2017-06-05T06:55:25Z

Description

Implements driver.ConnBeginTx, driver.QueryerContext, driver.ExecerContext, driver.ConnBeginTx interface
Implements driver.Pinger interface
Implements driver.StmtQueryContext, driver.StmtExecContext interface

This pull request is based on @bgaifullin's work in https://github.com/bgaifullin/mysql/commit/645810cb2dad3528d2b4be2b8432a898df348bbe , @edsrzf's work in #586, and @oscarzhao's work in #551.

refer to #496

Versus other implementation

With support of cancelation

This pull request is based on @bgaifullin's work with support of cancelation.

More detailed error information

The users want to distinguish between cancellation error and network error.
For example, the following code should panic with context.Canceled.

// from TestContextCancelQueryRow in driver_go18_test.go
ctx, cancel := context.WithCancel(context.Background())
rows, _ := dbt.db.QueryContext(ctx, "SELECT v FROM test")
rows.Next()
if err := rows.Scan(&v); err != nil {
	panic(err)
}

cancel()
// make sure the driver receives cancel request.
time.Sleep(100 * time.Millisecond)

rows.Next()
// rows.Err() should be context.Canceled
if err := rows.Err(); err != context.Canceled {
	panic(err)
}

This behavior is similar to the net/http package.

package main

import (
	"context"
	"fmt"
	"io/ioutil"
	"log"
	"net/http"
	"net/http/httptest"
	"time"
)

func main() {
	log.SetFlags(log.LstdFlags | log.Lshortfile)
	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		w.Header().Set("Context-Type", "text/plain")
		w.WriteHeader(200)
		fmt.Fprint(w, "Hello, ")
		w.(http.Flusher).Flush()
		time.Sleep(time.Second)
		fmt.Fprint(w, "client\n")
	}))
	defer ts.Close()

	req, err := http.NewRequest("GET", ts.URL, nil)
	if err != nil {
		log.Fatal(err)
	}
	ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond)
	go func() {
		time.Sleep(500 * time.Millisecond)
		cancel()
	}()
	defer cancel()

	req = req.WithContext(ctx)
	res, err := http.DefaultClient.Do(req)
	if err != nil {
		log.Fatal(err)
	}

	// ioutil.ReadAll will fail with "context canceled" or "context deadline exceeded" error.
	greeting, err := ioutil.ReadAll(res.Body)
	res.Body.Close()
	if err != nil {
		log.Fatal(err)
	}

	fmt.Printf("%s", greeting)
}

No race condition

fixed the race condition of @bgaifullin's work (https://github.com/bgaifullin/mysql/commit/645810cb2dad3528d2b4be2b8432a898df348bbe#diff-7cd962f1d3aee9adf93d5c17b49f4530R98).
I've checked by my race condition checker for go-mysql-driver.

$ go run -race main.go -p 10 -d 60s
2017/06/05 15:10:45 starting benchmark: concurrency: 10, time: 1m0s, GOMAXPROCS: 4
2017/06/05 15:11:45 done benchmark: score 11092, elapsed 1m0.025765215s = 184.787315 / sec
2017/06/05 15:11:45 conn.PrepareContext: context canceled 4619
2017/06/05 15:11:45 tx.ExecContext: context canceled 759
2017/06/05 15:11:45 stmt.Scan: context canceled 692
2017/06/05 15:11:45 tx.Commit: context canceled 95
2017/06/05 15:11:45 tx.Commit: sql: Transaction has already been committed or rolled back 760
2017/06/05 15:11:45 stmt.Scan: sql: Rows are closed 5
2017/06/05 15:11:45 conn.Scan: context canceled 1498
2017/06/05 15:11:45 conn.BeginContext: context canceled 3933
2017/06/05 15:11:45 conn.ExecContext: context canceled 877
2017/06/05 15:11:45 stmt.ExecContext: context canceled 503
2017/06/05 15:11:45 conn.PingContext: context canceled 304
2017/06/05 15:11:45 invalid connection 3675
2017/06/05 15:11:45 conn.PingContext: driver: bad connection 2355
2017/06/05 15:11:45 conn.Scan: sql: Rows are closed 2

I referred to the implementation of the http package.

Support of Read Only Transaction

~~START TRANSACTION READ ONLY is supported from MySQL 5.7.~~
https://dev.mysql.com/doc/refman/5.7/en/innodb-performance-ro-txn.html

EDITED: I'll moved it to a separate PR.

Checklist

Code compiles correctly
Created tests which fail without the change (if possible)
All tests passing
Extended the README / documentation, if necessary
Added myself / the copyright holder to the AUTHORS file

julienschmidt · 2017-06-05T07:23:37Z

The approach looks awesomely clean!

Please make entries to the AUTHORS file for all persons that your code is based on and yourself.

julienschmidt

My other concern is, that it introduces rather heavy synchronization at many places.

Do you see any potential to reduce the synchronization anywhere?

julienschmidt · 2017-06-05T07:26:06Z

connection.go

-func (mc *mysqlConn) cleanup() {
+func (mc *mysqlConn) cleanup(err error) {
+	if err == nil {
+		panic("nil error")


This should probably be a bit more descriptive

julienschmidt · 2017-06-05T07:29:03Z

connection.go

 		err = mc.writeCommandPacket(comQuit)
 	}

-	mc.cleanup()
+	mc.cleanup(errors.New("mysql: connection is closed"))


Might a user see this error?
If yes, please define it in errors.go

If it is completely internal, please define it at the top of the file.

julienschmidt · 2017-06-05T07:31:18Z

connection.go

 	"time"
 )

+//a copy of context.Context from Go 1.7 and later.


s/from/for/

and start the comment with a space please

julienschmidt · 2017-06-05T07:33:28Z

connection.go

+	finished         chan<- struct{}
+
+	mu          sync.Mutex // guards following fields
+	closed      error      // set non-nil when conn is closed, before closech is closed


this field has type error but is basically just used as a bool/flag. Please just use a bool instead.

julienschmidt · 2017-06-05T07:34:30Z

connection.go

+	mc.mu.Lock()
+	mc.canceledErr = err
+	mc.mu.Unlock()
+	mc.cleanup(errors.New("mysql: query canceled"))


same as for the other error

julienschmidt · 2017-06-05T07:38:47Z

rows.go

@@ -64,12 +65,24 @@ func (rows *mysqlRows) Columns() []string {
 	return columns
 }

+func (rows *mysqlRows) setFinish(f func()) {


just set is directly. This is not Java

This is implementation of setfinish interface that defined in connection_go18.go.
Of course, I can rewrite it setting directly, but QueryContext will be a little complex.

// in QueryContext switch r := rows.(type) { case mysqlRows: r.finish = mc.finish case binaryRows: r.finish = mc.finish case textRows: r.finish = mc.finish default: mc.finish() }

Now implementation is here.

// in QueryContext if set, ok := rows.(setfinish); ok { set.setFinish(mc.finish) } else { mc.finish() }

If we also consider the interface and function definition, we don't save anything here but introduce some extra overhead and complexity.

mc.startWatch and mc.finish() must be paired, but mysqlRows is used internally.

getWarnings: https://github.com/go-sql-driver/mysql/blob/v1.3/errors.go#L92

So, I usesetfinish to prevent mc.finish() from being called unexpectedly.

Here is another solution.

shogo82148@9b979f2

Which do you like?

ok. I'll cherry-pick shogo82148/mysql@9b979f2.

shogo82148 · 2017-06-05T07:52:05Z

Thanks. I added myself and the other persons.
I'll fix changes requested.

- s/from/for/ - and start the comment with a space please

shogo82148 · 2017-06-05T13:21:50Z

The benchmark result (EC2 c4.large and RDS Aurora db.r3.xlarge)

$ ~/go/bin/benchcmp old.txt new.txt 
benchmark                     old ns/op     new ns/op     delta
BenchmarkQueryContext/1-2     102848        103137        +0.28%
BenchmarkQueryContext/2-2     54461         56205         +3.20%
BenchmarkQueryContext/3-2     39958         40097         +0.35%
BenchmarkQueryContext/4-2     31477         32254         +2.47%
BenchmarkExecContext/1-2      103821        103121        -0.67%
BenchmarkExecContext/2-2      55591         55437         -0.28%
BenchmarkExecContext/3-2      39455         40441         +2.50%
BenchmarkExecContext/4-2      32416         32988         +1.76%
BenchmarkQuery-2              26059         30051         +15.32%
BenchmarkExec-2               99030         102532        +3.54%
BenchmarkRoundtripTxt-2       343523        337323        -1.80%
BenchmarkRoundtripBin-2       339690        342116        +0.71%
BenchmarkInterpolation-2      833           837           +0.48%
BenchmarkParseDSN-2           9254          9444          +2.05%

benchmark                     old allocs     new allocs     delta
BenchmarkQueryContext/1-2     21             22             +4.76%
BenchmarkQueryContext/2-2     21             22             +4.76%
BenchmarkQueryContext/3-2     21             22             +4.76%
BenchmarkQueryContext/4-2     21             22             +4.76%
BenchmarkExecContext/1-2      21             22             +4.76%
BenchmarkExecContext/2-2      21             22             +4.76%
BenchmarkExecContext/3-2      21             22             +4.76%
BenchmarkExecContext/4-2      21             22             +4.76%
BenchmarkQuery-2              22             23             +4.55%
BenchmarkExec-2               3              3              +0.00%
BenchmarkRoundtripTxt-2       15             16             +6.67%
BenchmarkRoundtripBin-2       17             18             +5.88%
BenchmarkInterpolation-2      1              1              +0.00%
BenchmarkParseDSN-2           63             63             +0.00%

benchmark                     old bytes     new bytes     delta
BenchmarkQueryContext/1-2     697           731           +4.88%
BenchmarkQueryContext/2-2     696           728           +4.60%
BenchmarkQueryContext/3-2     696           728           +4.60%
BenchmarkQueryContext/4-2     696           728           +4.60%
BenchmarkExecContext/1-2      696           728           +4.60%
BenchmarkExecContext/2-2      696           728           +4.60%
BenchmarkExecContext/3-2      696           728           +4.60%
BenchmarkExecContext/4-2      696           728           +4.60%
BenchmarkQuery-2              713           745           +4.49%
BenchmarkExec-2               67            72            +7.46%
BenchmarkRoundtripTxt-2       15907         15940         +0.21%
BenchmarkRoundtripBin-2       663           695           +4.83%
BenchmarkInterpolation-2      176           176           +0.00%
BenchmarkParseDSN-2           6896          6896          +0.00%

bgaifullin · 2017-06-05T13:39:48Z

connection.go

+	Deadline() (deadline time.Time, ok bool)
+	Done() <-chan struct{}
+	Err() error
+	Value(key interface{}) interface{}


If method Value is not used, you can drop it. It is not necessary to describe all methods of interface context.Context.

bgaifullin · 2017-06-05T13:45:51Z

connection.go

+	finished         chan<- struct{}
+
+	mu          sync.Mutex // guards following fields
+	closed      bool       // set true when conn is closed, before closech is closed


IMHO: it will be better to use atomic variable for closed, because this field is used often than canceledError. I believe it will be faster than acquire lock for each time.
in this case mu will guard only for canceledErr field.

Very good point, but Go currently doesn't have any atomic bool type in the standard library.
I would send another PR with an implementation based e.g. on uint32 to replace it after this is merged.

bgaifullin · 2017-06-05T13:53:25Z

connection_go18.go

+	"errors"
+)
+
+type setfinish interface {


Maybe finalizer ?

type finalizer interface { finalize(f func()) }

-1
because setFinish doesn't finalize, but the function f does.
Another suggestion.

type setterFinalizer interface { setFinalizer(finalizer func()) }

Are there other better names?

bgaifullin · 2017-06-05T14:04:03Z

rows.go

@@ -167,7 +186,10 @@ func (rows *textRows) NextResultSet() (err error) {

 func (rows *textRows) Next(dest []driver.Value) error {
 	if mc := rows.mc; mc != nil {
-		if mc.netConn == nil {
+		if mc.isBroken() {


What about moving this logic into method of connection.

if err := mc.error(); err != nil { return err }

where mc.error is

if mc.isBroken() { if err := mc.canceled(); err != nil { return err } return ErrInvalidConn } return nil

bgaifullin · 2017-06-05T14:06:55Z

driver_go18_test.go

+		}
+		if err := rows.Err(); err != context.Canceled {
+			dbt.Errorf("expected context.Canceled, got %v", err)
+		}


IMHO: it also be good to add check that there is no hanged gorutines. the number of gorutines, before QueryContext, should be less or equal number of gorutines after closing connection.

methane · 2017-06-05T15:16:33Z

When benchmarking, local MySQL is better than remote one, because network latency hides real performance.
If you want to simulate real workload, RTT [ms] * 100 concurrent connections may be needed to saturate CPU.

methane · 2017-06-05T15:17:59Z

I'm +1 for general design. I'm looking more carefully about race condition.

julienschmidt · 2017-06-05T15:47:16Z

And add a section to the README please ("context.Context support", just like "time.Time support)". Even if it just says that it is supported

julienschmidt · 2017-06-05T16:28:15Z

If possible, please also move the support for read-only transactions to a separate PR. This needs more consideration, as it is not supported by all servers.

edsrzf

Thanks for doing this work!

edsrzf · 2017-06-05T20:19:35Z

connection_go18.go

+}
+
+func (mc *mysqlConn) startWatcher() {
+	watcher := make(chan mysqlContext, 1)


You should create watcher and finished before starting this goroutine. If you initialize them here, then there's technically a race between this goroutine and other methods that use those channels.

there's technically a race between this goroutine and other methods that use those channels.

Yes, you are right.
But the watcher gorotuine starts in the end of startWatcher function, and watcher and finished are initialized before starting the watcher gorotuine.
so I think that there is no problems.

Ah, correct again. I misread this and thought that startWatcher was called as go mc.startWatcher() for some reason.

edsrzf · 2017-06-05T20:22:24Z

connection_go18.go

+}
+
+func (mc *mysqlConn) watchCancel(ctx context.Context) error {
+	select {


Maybe check if ctx.Done() == nil and if so, bail early? Seems like it would be a fairly common case and would allow you to save the overhead of channel synchronization and the overhead of defer mc.finish, which you'll have to do only under the same condition.

Thanks. I fixed it.

My MEMO:

Begin() uses ctx.Done() == context.Background().Done() for same reason.
https://github.com/golang/go/blob/go1.8.3/src/database/sql/ctxutil.go#L110

I checked which one is better.

$ ag -Q '.Done() == nil' src/context/context.go 243: if parent.Done() == nil { src/net/cgo_unix.go 81: if ctx.Done() == nil { 208: if ctx.Done() == nil { 223: if ctx.Done() == nil { 263: if ctx.Done() == nil { src/net/http/h2_bundle.go 6414: if req.Cancel == nil && ctx.Done() == nil { $ ag -Q '.Done() == context.Background()' src/database/sql/ctxutil.go 110: if ctx.Done() == context.Background().Done() {

ctx.Done() == nil wins.

edsrzf · 2017-06-05T20:57:10Z

connection_go18.go

+}
+
+func (mc *mysqlConn) startWatcher() {
+	watcher := make(chan mysqlContext, 1)


I also have some reservations about all connections sharing this same watcher goroutine.

On one hand, it's good that there's a single goroutine for doing this and we can avoid the overhead of goroutine creation every time we're running a query or whatever.

On the other, it means that only one connection can be watched at a time. And because the watcher channel has a buffer size of 1, only two connections can be running queries at the same time. You could increase the buffer size of watcher, but even if you do that only one connection will be watched for cancellation at a time.

For example, if you have an intentionally long-running query with a long timeout that's on the watched connection and you also have an unintentionally long-running query that sits in the watcher buffer and will be canceled, the one sitting in the buffer has to wait until the first query finishes before it can realize that it's run too long and should be canceled.

I'm not sure what concrete suggestions to offer here, but I think it's worth mentioning.

there is watcher per connection, not one watcher for all connections, isn't it?
Since mysql does not support multiplexing, only one query can be executed by the connection, so there is no problems IMHO.

there is watcher per connection, not one watcher for all connections, isn't it?

Yes, it is.
The database/mysql package handles multiple connections, so mysqlConn doesn't need to support multiplexing.
One watcher just watches one connection.

You're right! My mistake. Disregard this comment.

shogo82148 · 2017-06-05T22:42:37Z

Thank you for your comments!
I'll check them in this week.

julienschmidt · 2017-06-06T05:43:56Z

connection_go18.go

+	}()
+}
+
+func namedValueToValue(named []driver.NamedValue) ([]driver.Value, error) {


This should be in utils_go18.go

julienschmidt · 2017-06-06T08:40:27Z

The interface name suggested here #608 (comment) makes more sense to me and should be changed.
Otherwise the PR seems OK to me now.

Waiting for @methane, @bgaifullin, @edsrzf to approve it too now.

edsrzf

LGTM

julienschmidt · 2017-06-08T15:11:06Z

@shogo82148 please rename the interface

@methane, @bgaifullin please comment if you request any other changes or give your OK

bgaifullin · 2017-06-08T15:24:47Z

@julienschmidt I am waiting resolving for this 2 comments:
#608 (comment)
#608 (comment)

Otherwise the PR seems OK to me.

methane

@shogo82148 Good job!

methane · 2017-06-08T19:34:58Z

I confirmed watchCancel() and finish() are well designed.
You're goroutine concurrency expert!

shogo82148 · 2017-06-08T23:18:57Z

@methane Thanks a lot!

@bgaifullin I see. I'll fix in a moment.

shogo82148 · 2017-06-08T23:33:57Z

@bgaifullin I resolved for the 2 comments, please check it.

bgaifullin

@shogo82148 thank you.

LGTM

Improves #608

julienschmidt · 2017-06-09T19:41:53Z

PR #612 proposed a small update to this PR by replacing the remaining lock with an atomic variable and introducing wrapper structs for those.

* Add atomic wrappers for bool and error Improves #608 * Drop Go 1.2 and Go 1.3 support * "test" noCopy.Lock()

arvenil · 2018-09-21T14:56:33Z

@shogo82148 would you mind looking at #858 ?

Add supports to context.Context

7e64321

julienschmidt requested changes Jun 5, 2017

View reviewed changes

add authors related context.Context support

54ef181

shogo82148 added 2 commits June 5, 2017 16:53

fix comment of mysqlContext.

66fa137

- s/from/for/ - and start the comment with a space please

closed is now just bool flag.

06b17e6

bgaifullin reviewed Jun 5, 2017

View reviewed changes

julienschmidt added enhancement Go1.8 labels Jun 5, 2017

julienschmidt added this to the v1.4 milestone Jun 5, 2017

julienschmidt requested a review from methane June 5, 2017 15:11

julienschmidt changed the title ~~Add supports to context.Context~~ Add support of context.Context Jun 5, 2017

julienschmidt changed the title ~~Add support of context.Context~~ Add support for context.Context Jun 5, 2017

This was referenced Jun 5, 2017

Add supports to context.Context #551

Closed

Add context support #586

Closed

edsrzf suggested changes Jun 5, 2017

View reviewed changes

shogo82148 added 6 commits June 6, 2017 09:29

drop read-only transactions support

1705550

remove unused methods from mysqlContext interface.

41940ff

moved checking canceled logic into method of connection.

fd8a559

add a section about context.Context to the README.

a464739

use atomic variable for closed.

1fdad70

short circuit for context.Background()

f96feaa

julienschmidt reviewed Jun 6, 2017

View reviewed changes

julienschmidt approved these changes Jun 6, 2017

View reviewed changes

edsrzf approved these changes Jun 6, 2017

View reviewed changes

methane approved these changes Jun 8, 2017

View reviewed changes

shogo82148 added 2 commits June 9, 2017 08:21

rename watcher to starter.

85f33dd

use mc.error() instead of duplicated logics.

80f3f6f

bgaifullin approved these changes Jun 9, 2017

View reviewed changes

julienschmidt merged commit 5622634 into go-sql-driver:master Jun 9, 2017

julienschmidt added a commit that referenced this pull request Jun 9, 2017

Add atomic wrappers for bool and error

1d79804

Improves #608

julienschmidt mentioned this pull request Jun 9, 2017

Add atomic wrappers for bool and error #612

Merged

5 tasks

This was referenced Jun 9, 2017

Implement Context Interface #496

Closed

Implement Pinger interface #505

Closed

shogo82148 deleted the context-support branch June 9, 2017 23:15

marcin-ptaszynski mentioned this pull request Jun 9, 2017

ESI-11623 perform locking fetch on Delete operation cloudwan/gohan#477

Closed

julienschmidt added a commit that referenced this pull request Jun 10, 2017

Add atomic wrappers for bool and error (#612)

72e0ac3

* Add atomic wrappers for bool and error Improves #608 * Drop Go 1.2 and Go 1.3 support * "test" noCopy.Lock()

shogo82148 mentioned this pull request Jun 11, 2017

support READ-ONLY transactions #618

Merged

5 tasks

SuperQ mentioned this pull request Jun 16, 2017

Use native DB.Ping() instead of self-written implementation prometheus/mysqld_exporter#210

Merged

jsha mentioned this pull request Jun 20, 2017

Start using Contexts for database access letsencrypt/boulder#2819

Closed

knadh mentioned this pull request Jan 5, 2018

Does context cancellation cancel the underlying query? #731

Closed

This was referenced Jun 17, 2018

Drop Go 1.7 Support #823

Merged

Implement Connector and DriverContext interface #778

Closed

pjebs mentioned this pull request Oct 12, 2019

Feature Request: Add mysql context cancellation ent/ent#87

Closed

jsha mentioned this pull request Nov 14, 2019

Update github.com/go-sql-driver/mysql to latest tagged release letsencrypt/boulder#4530

Closed

Add support for context.Context #608

Add support for context.Context #608

Conversation

shogo82148 commented Jun 5, 2017 • edited Loading

Description

Versus other implementation

With support of cancelation

More detailed error information

No race condition

Support of Read Only Transaction

Checklist

julienschmidt commented Jun 5, 2017

julienschmidt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shogo82148 commented Jun 5, 2017

shogo82148 commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienschmidt Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

methane commented Jun 5, 2017

methane commented Jun 5, 2017

julienschmidt commented Jun 5, 2017

julienschmidt commented Jun 5, 2017

edsrzf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shogo82148 commented Jun 5, 2017

Choose a reason for hiding this comment

julienschmidt commented Jun 6, 2017

edsrzf left a comment

Choose a reason for hiding this comment

julienschmidt commented Jun 8, 2017

bgaifullin commented Jun 8, 2017

methane left a comment

Choose a reason for hiding this comment

methane commented Jun 8, 2017

shogo82148 commented Jun 8, 2017

shogo82148 commented Jun 8, 2017

bgaifullin left a comment

Choose a reason for hiding this comment

julienschmidt commented Jun 9, 2017

arvenil commented Sep 21, 2018

shogo82148 commented Jun 5, 2017 •

edited

Loading

julienschmidt Jun 5, 2017 •

edited

Loading