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

Jump to bottom

proposal: database/sql/driver: add OpenContext #22713

Closed
xiaost opened this issue Nov 14, 2017 · 5 comments
Closed

proposal: database/sql/driver: add OpenContext #22713

xiaost opened this issue Nov 14, 2017 · 5 comments
Labels
FrozenDueToAge Proposal
Milestone
Proposal

Comments

@xiaost
Copy link

xiaost commented Nov 14, 2017

functions, like QueryContext, ExecContext, may emits a driver.Open without context param.
it may causes application hang in *Context func even if the context was cancelled.

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

go version go1.9.2 linux/amd64

Does this issue reproduce with the latest release?

yes

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/xiaost/golang"
GORACE=""
GOROOT="/home/xiaost/golang/go"
GOTOOLDIR="/home/xiaost/golang/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build400501004=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

timeout -sABRT 1s go run t.go

package main

import (
    "context"
    "database/sql"
    "fmt"
    "net"
    "time"

    _ "github.com/go-sql-driver/mysql"
)

func main() {
    ln, _ := net.Listen("tcp", "localhost:0")
    go func() {
        for {
            conn, _ := ln.Accept()
            go func(conn net.Conn) { time.Sleep(time.Second * 10); conn.Close() }(conn)
        }
    }()
    dsn := fmt.Sprintf("username:password@tcp(%s)/dbname", ln.Addr())
    db, err := sql.Open("mysql", dsn)
    if err != nil {
        panic(err)
    }
    ctx, _ := context.WithTimeout(context.TODO(), 50*time.Millisecond)
    db.QueryContext(ctx, "SELECT * FROM GO")

}

What did you expect to see?

context deadline exceeded

What did you see instead?

timeout emits SIGABRT and panic in underlying driver`s Open
@gopherbot gopherbot added this to the Proposal milestone Nov 14, 2017
@ianlancetaylor
Copy link
Contributor

CC @kardianos

@kardianos
Copy link
Contributor

@xiaost Do you have direct production experience where this is a problem?

In the new driver connector https://tip.golang.org/pkg/database/sql/driver/#Connector, we ensure it takes a context parameter just like you are suggesting, but currently we are forced to ignore this parameter when using string DSN open method: https://go.googlesource.com/go/+/fa1f52c5f61fdda063851be16366b5eda5a08e58/src/database/sql/sql.go#600 .

I would see if we can put this in go1.10 as it can mirror the additional driver.Connector already added (drivers could support both variants at the same time) if we decide to do this.

@gopherbot
Copy link

Change https://golang.org/cl/77550 mentions this issue: database/sql: allow OpenConnector in a driver.Driver interface

@kardianos
Copy link
Contributor

I've mailed a CL for this proposal. After trying to implement it I'm actually very happy with the result and would recommend we merge it into go1.10 as it will complement the driver.Connector nicely (also added in go1.10.

Rather then just creating an OpenContext(ctx context.Context, name string) optional method, I create an interface that returns a driver.Connector. Not only do we get the context as requested in this issue, we also gain the DSN is only parsed once and it is parsed before the database pool is created (not on first ping). This will be more efficient, propagate context regardless of how the DB was opened, and return DSN parse errors sooner.

I see this as a strong win and buys the extra interface it adds.

@xiaost
Copy link
Author

xiaost commented Nov 14, 2017 •
edited

@kardianos awesome! I think OpenConnector is more friendly with driver developer for compatibility.

In production, sometimes we found that max latency of query up to one minute with a seconds-timeout context.
It is easy to found out that the routine hangs in driver.Open function and returns after dial or read/write timeout (read/write happens in connection initialization).

@golang golang locked and limited conversation to collaborators Nov 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
4 participants
@kardianos @ianlancetaylor @xiaost @gopherbot