Add context support #586

edsrzf · 2017-05-08T02:35:10Z

Description

This pull request is based on @oscarzhao's work in #551, but uses a
different method of achieving backward compatibility with older Go versions
to reduce duplicated code. oscarzhao is still listed as the commit author for the first commit.

For backward compatibility with Go versions < 1.8, the file ctx_backport.go
contains a redefinition of context.Context as well as a few other things.
ctx_go18.go contains definitions that simply alias the actual standard library
definitions. The result is that the vast majority of the code can be shared
amongst Go versions.

This PR also adds context support to the read side of things, which wasn't in #551.

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

For backward compatibility with Go versions < 1.8, the file ctx_backport.go contains a redefinition of context.Context as well as a few other things. The result is that the vast majority of the code can be shared between Go versions.

methane · 2017-05-08T12:20:38Z

packets.go

+	}
+	ctxDeadline, isCtxDeadlineSet := ctx.Deadline()
+	if isCtxDeadlineSet && !ctxDeadline.After(time.Now()) {
+		return errors.New("timeout")


Could we use context. DeadlineExceeded?
https://golang.org/pkg/context/#pkg-variables

methane · 2017-05-08T12:26:30Z

I like this approach.
Would you implement context support for reading side like writePacket()?
Or do you like to merge this and implement reading side later?

edsrzf · 2017-05-08T20:47:22Z

@methane I've updated to address your comment and have implemented the read side in a similar way. I also reworked the deadline logic for writing a bit so that context deadline always overrides write timeout.

Please take another look.

bgaifullin · 2017-05-09T09:06:44Z

This PR does not cover context.CancelContext.
And there is no tests for new methods.

methane · 2017-05-09T10:40:30Z

@bgaifullin Yes, I know. Full Context support is very large and hard job.
Before trying it, we need basic design for adding Context support without
massive duplicated code or dropping Go ~1.6.
And this pull request is the best I have ever seen about it.
Real cancellation can be implemented later.

bgaifullin · 2017-05-09T10:52:24Z

@methane I am afraid, that real cancelation will require rework all this stuff again.
I try to do it in my own fork.
first approach was the same like in this PR, after that I tried to add support for cancelation and I had to totally rework my first approach.

there is first approach (without support of cancelation): https://github.com/bgaifullin/mysql/tree/support_context
there is second( with support of cancelation): https://github.com/bgaifullin/mysql/commits/support_context2

edsrzf · 2017-05-09T21:29:01Z

@bgaifullin Alright, you do make a fair point. Implementing cancellation basically gives deadline support automatically, and there is some cleanup that should be done upon timeout or cancellation since the connection ends up in an unknown state and shouldn't be used anymore.

That said, I think it would be possible to implement cancellation at a lower level than you have in your support_context2 branch.

There are also questions about what it would mean to cancel. If we cancel a query, does it mean that we simply stop waiting for it to finish, or should we actively go in and try to kill the query too?

The other question is, how long would it take for us to figure this out? Does this branch represent enough of an improvement that we could just merge it and then still rework it? It satisfies my needs, but I understand if that's not enough.

julienschmidt · 2017-05-10T11:00:44Z

README.md

@@ -418,6 +418,11 @@ Version 1.0 of the driver recommended adding `&charset=utf8` (alias for `SET NAM

 See http://dev.mysql.com/doc/refman/5.7/en/charset-unicode.html for more details on MySQL's Unicode support.

+## Context Support
+Since go1.8, context is introduced to `database/sql` for better control on timeout and cancellation.
+New interfaces such as `driver.QueryerContext`, `driver.ExecerContext` are introduced. See more details on [context support to database/sql package](https://golang.org/doc/go1.8#database_sql, "sql").


The interfaces are driver implementation internals which do not interest the average driver user

julienschmidt · 2017-05-10T11:00:58Z

README.md

+Since go1.8, context is introduced to `database/sql` for better control on timeout and cancellation.
+New interfaces such as `driver.QueryerContext`, `driver.ExecerContext` are introduced. See more details on [context support to database/sql package](https://golang.org/doc/go1.8#database_sql, "sql").
+
+In Go-MySQL-Driver, we implemented these interfaces for structs `mysqlConn`, `mysqlStmt` and `mysqlTx`.


edsrzf · 2017-05-10T20:39:40Z

@julienschmidt README updated. Any thoughts on the pull request overall and the rest of the conversation?

coveralls · 2017-05-10T20:43:58Z

Changes Unknown when pulling 6d7e804 on edsrzf:context-support into ** on go-sql-driver:master**.

julienschmidt · 2017-05-11T06:06:55Z

@edsrzf I like the simplicity of the approach. @methane is our expert in that regard, I'll leave decisions regarding context support to him.

methane · 2017-05-11T12:30:34Z

@julienschmidt I think adding context support is risky task. I want to release bug fixes
without context support for the meanwhile.
Is it OK to adding new context branch to keep master branch stable?

edsrzf · 2017-05-11T22:49:54Z

I'm happy to change the base branch on this PR if that's what you want to do. Once you've decided, just create it and let me know. (Or probably you can just change it yourself.)

wooparadog · 2017-05-12T04:15:19Z

As a down stream user, we're currently building a tracing mechanism for our golang applications. And context support is a critical part where we want to plant identity information(Will generate sql comment when sending queries).

We'll try forking with this pull request in our code, and will post here any problem/fix if encountered. but it'll be fantastic if we can use the upstream directly.

wooparadog · 2017-05-12T04:38:18Z

connection.go

+	return mc.ExecContext(backgroundCtx(), query, args)
+}
+
+func (mc *mysqlConn) ExecContext(ctx mysqlContext, query string, args []driver.Value) (driver.Result, error) {


The interface defined in sql/driver is ExecContext(ctx context.Context, query string, args []NamedValue) (Result, error)

note the driver.Value is changed to driver.NamedValue

That's why there should be simple initialization tests, to check if our driver implements the respective interfaces, i.e. var _ driver.Interface = mysql.Implementation{}.

Here is an example of what I mean: e1cf7db

julienschmidt · 2017-05-12T15:41:59Z

@methane I'd like to avoid having a second development branch. This PR should be carefully reviewed (I will also give it a detailed review tomorrow) and then merged to master. If we find any problems, we will try to fix it or revert the changes if we can't.
I'd view our master branch much like Go's "tip". We stabilize it and avoid big changes if we're nearing a release, which should then contain stable code. Our next release should definitely be about supporting the Go 1.8 features. So context support should land in the master branch and we should have ample time for possible bug reports.

edsrzf · 2017-05-13T09:40:01Z

Fixed the interface issues and added those compile-time interface implementation tests. Good catch and good idea.

edsrzf · 2017-05-23T20:42:00Z

Can we find a path forward on this? I see two possibilities:

Continue with what I currently have, more or less, and figure out cancellation later.
Rework everything to include cancellation from the get go.

Are there any other options? I can also break up the pull request into smaller pieces if that would help.

methane · 2017-05-24T07:36:54Z

I'm sorry, but I need more time to consider about this comment.

methane · 2017-05-24T07:38:36Z

I wish we can drop Go 1.6 support and read_timeout, write_timeout options before
adding context support. But sadly, it's impossible.

dgryski · 2017-05-24T07:58:59Z

@methane Once the standard App Engine runtime is upgraded to 1.8, I think it's reasonable to drop support for 1.6 and earlier from the default branch.

julienschmidt · 2017-06-05T15:42:01Z

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

Add context support

94e3ba7

For backward compatibility with Go versions < 1.8, the file ctx_backport.go contains a redefinition of context.Context as well as a few other things. The result is that the vast majority of the code can be shared between Go versions.

edsrzf mentioned this pull request May 8, 2017

Add supports to context.Context #551

Closed

5 tasks

methane reviewed May 8, 2017

View reviewed changes

Use context.DeadlineExceeded error

669fc71

edsrzf force-pushed the context-support branch from dac4e22 to d5f60b5 Compare May 9, 2017 02:52

edsrzf added 2 commits May 9, 2017 17:10

Add context support for reads

2abb4ae

Rework write deadline logic

691ec98

edsrzf force-pushed the context-support branch from d5f60b5 to 691ec98 Compare May 9, 2017 05:10

julienschmidt reviewed May 10, 2017

View reviewed changes

julienschmidt modified the milestone: v1.4 May 10, 2017

README improvements

6d7e804

julienschmidt assigned methane May 11, 2017

julienschmidt added enhancement Go1.8 labels May 11, 2017

wooparadog reviewed May 12, 2017

View reviewed changes

Add compile-time interface tests and fix method signatures

ce38c4f

shogo82148 mentioned this pull request Jun 5, 2017

Add support for context.Context #608

Merged

5 tasks

julienschmidt closed this Jun 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add context support #586

Add context support #586

edsrzf commented May 8, 2017 •

edited

methane May 8, 2017

methane commented May 8, 2017

edsrzf commented May 8, 2017

bgaifullin commented May 9, 2017 •

edited

methane commented May 9, 2017

bgaifullin commented May 9, 2017 •

edited

edsrzf commented May 9, 2017

julienschmidt May 10, 2017

julienschmidt May 10, 2017

edsrzf commented May 10, 2017

coveralls commented May 10, 2017 •

edited

julienschmidt commented May 11, 2017

methane commented May 11, 2017

edsrzf commented May 11, 2017

wooparadog commented May 12, 2017

wooparadog May 12, 2017

julienschmidt May 12, 2017

julienschmidt May 12, 2017

julienschmidt commented May 12, 2017

edsrzf commented May 13, 2017

edsrzf commented May 23, 2017

methane commented May 24, 2017

methane commented May 24, 2017 •

edited

dgryski commented May 24, 2017

julienschmidt commented Jun 5, 2017

Add context support #586

Add context support #586

Conversation

edsrzf commented May 8, 2017 • edited

Description

Checklist

methane May 8, 2017

Choose a reason for hiding this comment

methane commented May 8, 2017

edsrzf commented May 8, 2017

bgaifullin commented May 9, 2017 • edited

methane commented May 9, 2017

bgaifullin commented May 9, 2017 • edited

edsrzf commented May 9, 2017

julienschmidt May 10, 2017

Choose a reason for hiding this comment

julienschmidt May 10, 2017

Choose a reason for hiding this comment

edsrzf commented May 10, 2017

coveralls commented May 10, 2017 • edited

julienschmidt commented May 11, 2017

methane commented May 11, 2017

edsrzf commented May 11, 2017

wooparadog commented May 12, 2017

wooparadog May 12, 2017

Choose a reason for hiding this comment

julienschmidt May 12, 2017

Choose a reason for hiding this comment

julienschmidt May 12, 2017

Choose a reason for hiding this comment

julienschmidt commented May 12, 2017

edsrzf commented May 13, 2017

edsrzf commented May 23, 2017

methane commented May 24, 2017

methane commented May 24, 2017 • edited

dgryski commented May 24, 2017

julienschmidt commented Jun 5, 2017

edsrzf commented May 8, 2017 •

edited

bgaifullin commented May 9, 2017 •

edited

bgaifullin commented May 9, 2017 •

edited

coveralls commented May 10, 2017 •

edited

methane commented May 24, 2017 •

edited