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:40:15Z

connection.go

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


 		err = mc.writeCommandPacket(comQuit)
 	}

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


 	"time"
 )

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


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


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


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

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


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


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


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


@@ -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() {


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


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


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


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


This should probably be a bit more descriptive

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.

s/from/for/

and start the comment with a space please

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

same as for the other error

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.

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

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.

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?

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

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.

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.

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.

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.

Add supports to context.Context

Loading status checks…

7e64321

julienschmidt requested changes Jun 5, 2017

View changes

Sep	OCT	Nov
	20
2019	2020	2021

go-sql-driver / mysql

Join GitHub today

Add support for context.Context #608

Add support for context.Context #608

Conversation

shogo82148 commented Jun 5, 2017 • edited

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

This comment has been minimized.

julienschmidt Jun 5, 2017 Member This should probably be a bit more descriptive

This comment has been minimized.

julienschmidt Jun 5, 2017 Member 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.

This comment has been minimized.

julienschmidt Jun 5, 2017 Member s/from/for/

This comment has been minimized.

julienschmidt Jun 5, 2017 Member and start the comment with a space please

This comment has been minimized.

julienschmidt Jun 5, 2017 Member this field has type error but is basically just used as a bool/flag. Please just use a bool instead.

This comment has been minimized.

julienschmidt Jun 5, 2017 Member same as for the other error

This comment has been minimized.

julienschmidt Jun 5, 2017 Member just set is directly. This is not Java

This comment has been minimized.

This comment has been minimized.

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

This comment has been minimized.

shogo82148 Jun 6, 2017 Author Contributor 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.

This comment has been minimized.

shogo82148 Jun 6, 2017 Author Contributor Here is another solution. shogo82148@9b979f2 Which do you like?

This comment has been minimized.

julienschmidt Jun 6, 2017 Member Option 2

This comment has been minimized.

shogo82148 Jun 6, 2017 Author Contributor ok. I'll cherry-pick shogo82148/mysql@9b979f2.

shogo82148 commented Jun 5, 2017

shogo82148 commented Jun 5, 2017

This comment has been minimized.

bgaifullin Jun 5, 2017 Contributor If method Value is not used, you can drop it. It is not necessary to describe all methods of interface context.Context. 👍 1

This comment has been minimized.

bgaifullin Jun 5, 2017 Contributor 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. 👍 1

This comment has been minimized.

julienschmidt Jun 5, 2017 • edited Member 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.

This comment has been minimized.

bgaifullin Jun 5, 2017 Contributor Maybe finalizer ? type finalizer interface { finalize(f func()) } 👍 1

This comment has been minimized.

shogo82148 Jun 6, 2017 Author Contributor -1 because setFinish doesn't finalize, but the function f does. Another suggestion. type setterFinalizer interface { setFinalizer(finalizer func()) } Are there other better names?

This comment has been minimized.

bgaifullin Jun 5, 2017 Contributor 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 👍 1

This comment has been minimized.

bgaifullin Jun 5, 2017 Contributor 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 commented Jun 5, 2017

methane commented Jun 5, 2017

julienschmidt commented Jun 5, 2017

julienschmidt commented Jun 5, 2017

edsrzf left a comment

This comment has been minimized.

edsrzf Jun 5, 2017 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.

This comment has been minimized.

This comment has been minimized.

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

This comment has been minimized.

edsrzf Jun 5, 2017 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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bgaifullin Jun 5, 2017 Contributor 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.

This comment has been minimized.

shogo82148 Jun 6, 2017 Author Contributor 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.

This comment has been minimized.

edsrzf Jun 6, 2017 You're right! My mistake. Disregard this comment.

shogo82148 commented Jun 5, 2017

This comment has been minimized.

julienschmidt commented on connection_go18.go in f96feaa Jun 6, 2017

julienschmidt commented Jun 6, 2017

shogo82148 commented Jun 5, 2017 •

edited

julienschmidt Jun 5, 2017
Member

This should probably be a bit more descriptive

julienschmidt Jun 5, 2017
Member

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 Jun 5, 2017
Member

s/from/for/

julienschmidt Jun 5, 2017
Member

and start the comment with a space please

julienschmidt Jun 5, 2017
Member

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

julienschmidt Jun 5, 2017
Member

same as for the other error

julienschmidt Jun 5, 2017
Member

just set is directly. This is not Java

julienschmidt Jun 5, 2017
Member

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

shogo82148 Jun 6, 2017
Author Contributor

`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 use`setfinish` to prevent `mc.finish()` from being called unexpectedly.

shogo82148 Jun 6, 2017
Author Contributor

Here is another solution.

shogo82148@`9b979f2`

Which do you like?

julienschmidt Jun 6, 2017
Member

Option 2

shogo82148 Jun 6, 2017
Author Contributor

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

bgaifullin Jun 5, 2017
Contributor

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

bgaifullin Jun 5, 2017
Contributor

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.

julienschmidt Jun 5, 2017 •

edited

Member

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 Jun 5, 2017
Contributor

Maybe `finalizer` ?

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

shogo82148 Jun 6, 2017
Author Contributor

-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 Jun 5, 2017
Contributor

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 Jun 5, 2017
Contributor

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.

edsrzf Jun 5, 2017

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.

edsrzf Jun 6, 2017

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

edsrzf Jun 5, 2017

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.

bgaifullin Jun 5, 2017
Contributor

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.

shogo82148 Jun 6, 2017
Author Contributor

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.

edsrzf Jun 6, 2017

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

julienschmidt commented on `connection_go18.go` in `f96feaa` Jun 6, 2017