Add supports to context.Context

[oscarzhao]

Description

1. Implements driver.ConnBeginTx, driver.QueryerContext, driver.ExecerContext, driver.ConnBeginTx interface
2. Implements driver.Pinger interface
3. Implements driver.StmtQueryContext, driver.StmtExecContext interface

refer to issue 496

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

Build tag go1.8 is used to adapt to both version pre 1.8 and 1.8+, previous code is kept as it was to support go1.2~go1.7.

[methane]

Thank you for your efforts.

Before reviewing,

• Please use build tag, instead of dropping Go 1.7 support.
• Please split this to small pull requests. Maybe, (1) Pinger, (2) Query and Exec, (3) Transaction.

[oscarzhao]

@methane I know how to use build tag to make the code adapted to different operating systems automatically, but cannot figure out how to use build tag for adaption to different go compiler versions. The ideal state is that, developers just import the package into their code (with different go compiler versions), and don't need to do any extra work. Do you have any good idea for this?

[julienschmidt]

Use two files: one for versions pre 1.8 and one for version 1.8 and later.

The required build tags are:

// +build !go1.8

in the pre 1.8 file, and

// +build go1.8

for the 1.8+ file.

[oscarzhao]

@julienschmidt Thank you. Build tag go1.8 is added to support different versions of go compiler. Previous code remains no changed (except adding build tag) for readability and extensibility.

@methane About spliting the pull request into small ones, perhaps the job is not necessary, because the major code changes lies in mysqlConn.writePacket and mysqlConn.writeXXXPacketYYY functions, not in implementing interfaces defined in database/sql/driver.

[methane]

@oscarzhao I don't like !go1.8 build tag. Please avoid it when possible.
See https://github.com/lib/pq/blob/master/conn_go18.go and https://github.com/lib/pq/blob/master/conn.go.

[oscarzhao]

@methane The introduction of context.Context into github.com/go-sql-driver/mysql needs to change many underlying functions(such as mysqlConn.writePacket and mysqlConn.writeXXX) and exported functions(such as QueryContext, Query). However, the introduction of context.Context into github.com/lib/pq does not require the sql library to change a lot because it has cancel mechanism support.

Since the beginning of this pull request, I've been thinking about how to reuse existing code(for previous versions of golang), how to keep code's readability and maintainability. I've tried to reuse existing code as much as possible, which results to very bad readability, the logic becomes confusing and difficult to understand. That's why I keep all the existing code as it is for golang pre 1.8. In this way, I think the build tag !go1.8 is necessary for now.

PS:

An alternative solution is that we implements the cancel mechanism via COM_PROCESS_KILL command, and then add context support in the way similar to Postgres driver. I will try this solution asap.

[methane]

OK, I'll start review in this week.

[methane]

Don't decorate.

https://golang.org/doc/effective_go.html#commentary

Comments do not need extra formatting such as banners of stars.

[methane]

Waiting response should be cancellable.

[bgaifullin]

IMHO: PR contains a lot of code duplication, it will be hard to manage this code in future, because it will require to update each thing in 2 places.
All functions should be in one instance, and functions which does not accept context should just recall the function which accepts context by passing context.Background to them. I believe this may be implemented without any build conditions.

[bgaifullin]

I have done POC, you can found results here https://github.com/bgaifullin/mysql/tree/support_context

I cannot avoid file connection_go18.go, because driver.TxOptions, driver.NamedValue was introduced in go1.8. but I avoid code duplication and just reuse the same function like it has done in lib/pg

[methane]

@bgaifullin Thanks.
So, if we can drop Go ~1.6 support, we can reduce about 2000 lines of code....

[julienschmidt]

We're now at Go 1.8. I think supporting the 3 most recent Go versions should be sufficient, but we should discuss this in a separate issue then.

Edit: It seems like only the 2 most recent Go versions receive official support: https://golang.org/dl/

[julienschmidt]

And the current amount of code duplication in this PR is definitely not acceptable

[methane]

@julienschmidt How about adding new experimental "context" branch which supports only Go 1.7+?

• (master branch) v1.4, v1.5, ...: stable, backward compatible branch which doesn't support (or limited support) context. For example, compression support can be added.

• (context branch) v2.0: Support only Go 1.7+.

  Context is completely supported.
  And some backward incompatible changes can be happen. For example, some timeout options can be removed because context has timeout / deadline.

[bgaifullin]

copy-paste from issue:

What about using net.Conn.SetDeadline(time.Now()) in case if need to interrupt current request.
in this case the implementation can be similar to postures implementation with only one difference:
in implementation of method cancel:

func (mc *mysqlConn) cancel() {
   mc.netConn.SetDeadline(time.Now())
}

there is still problem with orphan queries on mysql.server, but client will interrupt any query as soon > as cancel on context is called.

there is POC https://github.com/bgaifullin/mysql/tree/support_context2

[zecke]

I am not a go expert and try to make use of the Context with a deadline. My code is doing..

ctx, cancel := context.WithTimeout(httpReq.Context(), 1 * time.Second)
db, _ := sql.Open("mysql", dsn)
err = db.PingContext(ctx)
...

And then I run a "broken" MySQL server using:

nc -l 3306

the operation does not time out. Is there any way to get PingContext to honor the Deadline set? This was tried with master and this pull request.

[julienschmidt]

That's actually a pretty good idea for a test we should add!

[zecke]

That would be great! After reading the code and the SQL package definition I wonder:

Open may just validate its arguments without creating a connection to the database. To verify that the data source name is valid, call Ping.

In the case of this driver Open will open a TCP connection to the driver and issue a blocking(?) call to mc.readInitPacket(). As Open doesn't take a context maybe this code should be moved to Ping/PingContext?

[edsrzf]

I've created PR #586 with basically the same code (credit given) that manages to get rid of the code duplication. Please take a look and see if perhaps it's a little more palatable.

[julienschmidt]

Closed in favor of #608. Please provide your feedback there.