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: driver.ErrBadConn masked in *sql.DB.Begin #4433

Closed
gopherbot opened this issue Nov 25, 2012 · 7 comments
Closed

database/sql: driver.ErrBadConn masked in *sql.DB.Begin #4433

gopherbot opened this issue Nov 25, 2012 · 7 comments
Assignees
Milestone

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 25, 2012

by jane.chalfant:

What steps will reproduce the problem?

1. Encounter driver.ErrBadConn when calling driver.Conn.Begin from within *sql.DB.begin
2. Observe that *sql.DB.Begin does not call *sql.DB.begin again up to 10 times, as
driver.ErrBadConn != fmt.Errorf("sql: failed to Begin transaction: %v",
driver.ErrBadConn)

What is the expected output?
Up to 9 subsequent attempts to call *sql.DB.begin()

What do you see instead?
Immediate failure after driver.ErrBadConn is encountered.

Which compiler are you using (5g, 6g, 8g, gccgo)?
6g

Which operating system are you using?
Mac OS X 10.8.2

Which version are you using?  (run 'go version')
1.0.3

Please provide any additional information below.

It appears to me that there database/sql intends to retry many driver operations
(Prepare, Exec, Begin) up to 10 times if a driver.ErrBadConn is encountered. This retry
logic doesn't get applied in *sql.DB.Begin, as *sql.DB.begin will create a new, more
informative error which then causes "err != driver.ErrBadConn" to resolve to
true.

Consider the following code, if ci.Begin() returns driver.ErrBadConn:

func (db *DB) Begin() (*Tx, error) {
    var tx *Tx
    var err error
    for i := 0; i < 10; i++ {
        tx, err = db.begin()
        if err != driver.ErrBadConn {
            break
        }
    }
    return tx, err
}

func (db *DB) begin() (tx *Tx, err error) {
    ci, err := db.conn()
    if err != nil {
        return nil, err
    }
    txi, err := ci.Begin()
    if err != nil {
        db.putConn(ci, err)
        return nil, fmt.Errorf("sql: failed to Begin transaction: %v", err)
    }
    return &Tx{
        db:  db,
        ci:  ci,
        txi: txi,
    }, nil
}

Note that the error returned from db.begin() will not be driver.ErrBadConn, it will be
the result of this:

fmt.Errorf("sql: failed to Begin transaction: %v", driver.ErrBadConn)

One possible solution would be to return driver.ErrBadConn
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Nov 25, 2012

Comment 1 by james.chalfant:

Awkwardly enough, I was signed in as the wrong google user when I entered this (Jane
Chalfant rather than James Chalfant)
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Nov 25, 2012

Comment 2 by james.chalfant:

Diff attached with proposed solution. 
If the this seems appropriate, I've also created a change at
http://golang.org/cl/6845094 that I can mail for review.

Attachments:

  1. sql.go.diff (336 bytes)
@rsc
Copy link
Contributor

@rsc rsc commented Dec 9, 2012

Comment 3:

Brad, want to look at the CL?

Labels changed: added priority-later, removed priority-triage.

Owner changed to @bradfitz.

Status changed to Accepted.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 10, 2012

Comment 4:

Labels changed: added size-m.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 13, 2012

Comment 5:

Seems good. Sorry for the delay.
Can you sign the CLA and then hg mail 6845094?
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Dec 13, 2012

Comment 6 by james.chalfant:

I've signed the CLA online and done ''hg mail 6845094".
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 13, 2012

Comment 7:

This issue was closed by revision 309eae1.

Status changed to Fixed.

@rsc rsc added this to the Go1.1 milestone Apr 14, 2015
@rsc rsc removed the go1.1 label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
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
3 participants
You can’t perform that action at this time.