Connector Interface, take 3

[vmg]

Description

Hi everyone! Here's another change from GitHub's internal fork of the MySQL driver. This is basically a rebase + refactoring of @shogo82148's original PR (#778) which has been stalled for almost a year!

This Pull Request brings the Go 1.0+ Connector interface to the MySQL driver, fixing several bugs, such as : in usernames DSNs, and most importantly for us, allowing new MySQL connections to obey a context.Context when dialing to the MySQL cluster.

This PR updates the original with all the changes that have happened to master since then, and fixes several bugs and missing features, namely:

• The DialContext registry has been implemented
• The behavior of NewConnector has been fixed so it's always equivalent to passing in a DSN
• Cloning of Config structs has been improved so it no longer panics in some circumstances

We've been running this branch internally for a while and it's been working great. I'm going to be actively checking in on this PR as much as needed to ensure it doesn't go stale. I would love to land it upstream because it's been a while since Go 1.10 has been released and I think the Driver should really support the new Connector interface.

Let me know how can I make this easier to land!

cc @methane @shogo82148

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

[vmg]

The Coverall metrics have been reduced because we're no longer testing the deprecated RegisterDialFunc. Would you like me to add tests for this deprecated function?

[methane]

@vmg Please note that we can't fix even critical race condition issue (#903) by stuck of the project.

I don't want to move forward leaving such a critical issue...
Race is really bad thing in Go.

[vmg]

Hi @methane, are you saying that merging this PR would trigger the issue in #903, and that without these changes, the issue does not trigger?

I don't quite understand that. This PR changes the interface to create new connections. How is that related to the rows.Close() issue?

[vmg]

I've been reading up on the issue some more, and I believe these changes are completely orthogonal to #903. This PR is backwards compatible with Go versions older than Go 1.10, and with or without this PR, the bug in #903 will take effect, so I don't see why that bug should block progression on the Connector interface.

[methane]

Hi @methane, are you saying that merging this PR would trigger the issue in #903, and that without these changes, the issue does not trigger?

No. I meant I want we maintainers focus on fixing critical issue,
before take time to implementing or reviewing new features.
Our resource is very limited.

[methane]

And it is very helpful if experienced contributors like you join discussion about
how we should fix #903.

[vmg]

I understand. I'll prepare a fix for #903 and submit it next week. Meanwhile, given that the issue is not related to this specific PR, I would appreciate if you could start reviewing these changes, since they are hardly new features but an important bug fix that users of the library are experiencing. Thanks.

[vmg]

@methane: I know you're very busy but I would appreciate some help reviewing this PR, now that the Rows.Close fix has been merged. 🙇

I've pushed more tests to this PR to ensure we're covering all corner cases. Just to emphasize: besides being a new feature, implementing the Connector interface fixes an important bug we've been seeing in production, where any Context query, e.g. rows, err := db.QueryContext(ctx, "...sql...") can block forever after the Context has been cancelled.

This bug happens because without the Connector interface, calls to QueryContext or ExecContext that need to create a new MySQL connection will just call (*MySQLDriver).Open(dsn string) -- and this method does not take a Context so it can block forever when dialing the MySQL server.

We've been working around this by adding a ?timeout= to the DSN, but the timeout is always the same, so it doesn't take into account queries where Context is about to expire. Implementing the Connector interface fixes all these issues.

Thanks again 🙇

[shogo82148]

Great work!
Thank you for improving my implementation.

[methane]

• The DialContext registry has been implemented

Why should it be implemented? With connector interface, we don't need any registries, do we?

Uh, current Config is bad designed. I'm OK to having DialContext registry for a while.

[vmg]

@methane: I think we still need RegisterDialContext to support custom network protocols. The Connector interface simply provides a Connect(ctx context.Context) method to the db/sql package that allows it to use a Context when opening a connection, as opposed to the old (*MySQLDriver).Open.

Note that (*Connector).Connect returns a driver.Conn, same as (*MySQLDriver).Open. So if the user wants to use a custom network protocol for the underlying net.Conn, they still need to provide a RegisterDialContext function.

[vmg]

@shogo82148 🙇

[vmg]

@methane: If you would prefer, I could implement a NewConnectorWithDialer function that sets a default dialer when you create a Connector. This way we would not need to implement RegisterDialContext. But I don't know what to do with the old RegisterDial API. It would still need to work, so NewConnectorWithDialer would need to check 2 places for dialers (the global registry and the local dialer).

[shogo82148]

In addition, our driver should implement driver.DriverContext.
There is no way to pass a dialer using this interface.

[methane]

@vmg I meant we can something like:

cfg := mysql.ParseDSN(dsn)
cfg.SetDialer(YourDialer)
cn, err := mysql.NewConnector(cfg)
db, err := sql.OpenDB(cn)

We need registries because we couldn't put objects in DSN.
Unlike config object, we can use objects in Config.

[vmg]

@methane: I see. That sounds good to me. I will implement it.

What should I do with the existing registry? Should I remove the API?

[vmg]

Question: what should happen when the user creates a DSN from a Config that has a dialer and uses the old API?

Example:

cfg := mysql.ParseDSN(dsn)
cfg.SetDialer(YourDialer)
db, err := sql.Open("mysql", cfg.FormatDSN())

Here cfg has a custom Dialer, but the dialer will be lost when calling (*Config).FormatDSN(). This is un-intuitive. Should FormatDSN panic if it has a Dialer, because it cannot serialize it?

[methane]

In addition, our driver should implement driver.DriverContext.
There is no way to pass a dialer using this interface.

When people want to use new feature (in this time, DialContext support), people should use sql.OpenDB.

But in this PR, I'm OK to add RegisterDialContext.
At some point (v2.0.0), we can remove registries.

I don't want to add more registries. But RegisterDialContext doesn't increase number of registry.

[vmg]

That sounds good to me. In 2.0 we can remove RegisterDial, RegisterDialContext, and change (*Config).FormatDSN() string to (*Config).FormatDSN() (string, error), so we never lose information when serializing.

[shogo82148]

That sounds good to me, but I think it is out of this pull request's scope.
how about creating a milestone for version 2.0, and discuss about it?