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

database/sql: db.PingContext takes longer to time out on certain database host IPs than on others (postgresql and mysql) #27476

Closed
adavea opened this issue Sep 3, 2018 · 6 comments

Comments

@adavea
Copy link

@adavea adavea commented Sep 3, 2018

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go1.10.1 linux/amd64

Does this issue reproduce with the latest release?

Yes, at least on go1.10.4 linux/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOHOSTARCH="amd64"
GOHOSTOS="linux"

What did you do?

Attempted to use context.WithTimeout to limit the amount of time a PingContext() command takes to try and access a database server.

package main

//Extended timeout will reproduce for both msyql and postgres

import (
	"context"
	"database/sql"
	"log"
	"time"

	//_ "github.com/go-sql-driver/mysql" //MySQL driver produces same issue
	_ "github.com/lib/pq" //Postgresql driver produces same issue
)

func main() {
	log.Print("Trying to ping database!")

	//Prepare a "context" to execute queries in, this will allow us to use timeouts
	var bgCtx = context.Background()
	var ctx2SecondTimeout, cancelFunc2SecondTimeout = context.WithTimeout(bgCtx, time.Second*2)
	defer cancelFunc2SecondTimeout()

	//Open  database connection
	//db, err := sql.Open("mysql", "root:@tcp(172.1.2.3)/testdb?parseTime=true")
	db, err := sql.Open("postgres", "host=172.1.2.3 user=a password=b dbname=c")
	if err != nil {
		log.Printf("Unable to open db, %s", err.Error())
		return
	}
	log.Print("Successfully called open()")

	//Ping database connection with 2 second timeout
	err = db.PingContext(ctx2SecondTimeout)
	if err != nil {
		log.Printf("Can't ping database server in order to use storage, %s", err.Error())
		return
	}
	log.Print("Successfully pinged database!")
}

What did you expect to see?

A timeout of 2- seconds for 172.1.2.3
A timeout of 2- seconds for 541.1.2.3

What did you see instead?

A timeout of 2+ minutes for 172.1.2.3
(An immediate timeout for 541.1.2.3, which is ok)

@kardianos
Copy link
Contributor

@kardianos kardianos commented Sep 3, 2018

541 is larger then 255 and invalid. Please ensure the drivers use the provided context when dialing. Maybe inspect the MySQL driver dial code or file a bug there.

@xiaoxubeii
Copy link
Contributor

@xiaoxubeii xiaoxubeii commented Sep 4, 2018

It seems that in https://github.com/golang/go/blob/master/src/database/sql/sql.go#L1177:

db.numOpen++ // optimistically
db.mu.Unlock()
ci, err := db.connector.Connect(ctx)

It won't respect ctx if make a connection request at last.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Sep 4, 2018

Ah yes, This isn't an issue of not supporting PingContext, these drivers need to support the new Connector interface because this is a dial timeout.

@andybons andybons changed the title db.PingContext takes longer to time out on certain database host IPs than on others (postgresql and mysql) database/sql: db.PingContext takes longer to time out on certain database host IPs than on others (postgresql and mysql) Sep 4, 2018
@andybons andybons added the NeedsFix label Sep 4, 2018
@andybons andybons added this to the Go1.12 milestone Sep 4, 2018
@kardianos
Copy link
Contributor

@kardianos kardianos commented Sep 4, 2018

@andybons There isn't actually a fix here for Go. I wasn't sure if I should leave this issue open as a tracking issue for drivers who need to implement the Connector interface. I haven't had time to look thorugh the various drivers yet.

@andybons andybons modified the milestones: Go1.12, Unreleased Sep 5, 2018
@andybons
Copy link
Member

@andybons andybons commented Sep 5, 2018

@kardianos I see. Feel free to keep open and use it as a tracking bug for other drivers if you want, though I don’t know what qualifies as “fixed” in this case.

Can you update the title to properly represent the issue?

@kardianos
Copy link
Contributor

@kardianos kardianos commented Sep 5, 2018

The mysql driver has two pull requests to support the Connector. Nothing merged as of yet.

The postgresql driver supports the connector interface, but discards the context passed in. https://github.com/lib/pq/blob/90697d60dd844d5ef6ff15135d0203f65d2f53b8/connector.go#L23

I'm closing this issue as there is nothing to be done.

@kardianos kardianos closed this Sep 5, 2018
@golang golang locked and limited conversation to collaborators Sep 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.