Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DialFunc needs a timeout parameter #250

Closed
pcekm opened this issue Jun 17, 2014 · 10 comments
Closed

DialFunc needs a timeout parameter #250

pcekm opened this issue Jun 17, 2014 · 10 comments

Comments

@pcekm
Copy link

pcekm commented Jun 17, 2014

The DSN contains a timeout parameter that is passed to the standard net.Dialer. Please provide this to custom dialers as well.

@arnehormann
Copy link
Member

Sorry for replying late.

Just set the timeout parameter yourself in the Dialfunc.
You provide and implement it, you can set a timeout.

If that's not possible, please tell us why.

@pcekm
Copy link
Author

pcekm commented Aug 11, 2014

For the same reason you provide an address parameter. A DialFunc is registered once for a given network type, but it's used to create multiple connections. Those connections may have different addresses and different timeouts.

@julienschmidt
Copy link
Member

The addr part of the DSN is not touched by the driver and passed as is to the dial function. One could include a timeout parameter in the addr part and extract the value in the custom dial function.
But this wouldn't be a nice solution IMO.

In case we modify the DialFunc type, should we just add a timeout parameter or should be do something more extensible, like passing a map somehow? How realistic is it, that a dialer needs access to another parameter?

@pcekm
Copy link
Author

pcekm commented Aug 13, 2014

A timeout parameter satisfies my purposes, although someone else could conceivably want the tls parameter. In any case, something more extensible might prevent the need to change the interface again. A struct containing some (or all) of the values parsed from the DSN would be better than a map, though, since each field would have the correct type and would appear in godoc with comments.

@arnehormann
Copy link
Member

If we do so, there's a potential synergy with #224.

Assuming

type Dsn struct {
    User string
    Password string
    Protocol string
    Address string
    Schema string
    // enforce and clarify that every parameter only appears once and is position independent
    Params map[string]string
}

We just move the logic in MySQLDriver.Open(dsn string) to Dsn.Open() and export a new ParseDsn(dsn string).
MySQLDriver.Open(dsn string) just calls ParseDsn(dsn).Open().

@Freeaqingme
Copy link

Freeaqingme commented Dec 9, 2015

I understand this is not a trivial issue to fix, but what are we going to do with it? Personally I feel my proposed patch would be a good compromise for now, while we ponder some more on the desired solution for how we handle some of the magic that's been currently implemented.

Edit: I think this should have been posted as part of #346 not this thread...

@methane
Copy link
Member

methane commented Oct 27, 2016

Go 1.8 adds Context support to database/sql.
When we support Context, I think option is not necessary. Right?

@pcekm
Copy link
Author

pcekm commented Oct 27, 2016

It doesn't change this issue. Context support is being added for database operations such as Exec and Prepare. The timeout parameter determines how long to try opening a connection to the backend (i.e. Driver.Open), and there still won't be a context on that.

@julienschmidt julienschmidt added this to the v1.5 milestone Oct 7, 2017
@shogo82148
Copy link
Contributor

I implemented the driver.DriverContext interface in #778,
it allows to cancel the context during handshaking.
And I'm going to implement RegisterDialContext
( shogo82148@89cc76d ).

They solves this issue, doesn't them?

@methane methane closed this as completed Apr 4, 2019
@methane
Copy link
Member

methane commented Apr 4, 2019

DialContext is supported now.
You can use custom dialer function too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants