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: assumes that driver.Stmt.Close does not need the connection #5283

Closed
gopherbot opened this issue Apr 13, 2013 · 4 comments
Closed
Assignees

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 13, 2013

by awpritchard:

go version: the particular manifestation of this I'm hitting reproduces from
231af8ac63aa (scheduler changes that almost certainly are only revealing the problem,
not causing it) through tip (9bb42a7021c9).

see database/sql/sql.go:1308:
> func (rs *Rows) Close() error {
>    if rs.closed {
>        return nil
>    }
>    rs.closed = true
>    err := rs.rowsi.Close()
>    rs.releaseConn(err)
>    if rs.closeStmt != nil {
>        rs.closeStmt.Close()
>    }
>    return err
> }

This puts the Rows' connection back into the idle pool, and then calls the
driver.Stmt.Close method of the Stmt it belongs to.  In the postgresql driver
implementation (https://github.com/lib/pq), Stmt.Close communicates with the server (on
the connection that was just put back into the idle pool).  Most of the time, this
causes no problems, but if another goroutine makes a query at the right (wrong?) time,
chaos results.

Providing steps to reproduce is difficult, since it would have to involve multiple
packages and force a race condition to occur.

Moving the call to releaseConn to after the Stmt.Close fixes the problem in my case;
but, there are other places that Close a Stmt after releasing its connection.

In any case, traffic is being sent on "free" connections shortly after they
are freed, leading to race conditions that kill the driver code.

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

Which operating system are you using?
64-bit Linux, 3.7.7

Which version are you using?  (run 'go version')
go version devel +9bb42a7021c9 Thu Apr 11 13:40:14 2013 -0700 linux/amd64

Please provide any additional information below.
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 13, 2013

Comment 1:

Owner changed to @bradfitz.

Status changed to Accepted.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 15, 2013

Comment 2:

Thanks!
Fix and test out for review at https://golang.org/cl/8633044
You said "there are other places that Close a Stmt after releasing its connection" ...
where are those?  I didn't see any in a quick pass over the code.
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Apr 15, 2013

Comment 3 by awpritchard:

The other place I noticed was in (*DB).queryConn, and I was suspicious of
(*DB).noteUnusedDriverStmt, although on closer inspection it seems to be guaranteed that
the connection cannot be acquired by another thread in that case.  I didn't want to
claim to have an exhaustive list and in fact be missing some, though; those are just all
the cases I'm aware of.
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 15, 2013

Comment 4:

This issue was closed by revision 36d3bef.

Status changed to Fixed.

@mikioh mikioh changed the title database/sql assumes that driver.Stmt.Close does not need the connection database/sql: assumes that driver.Stmt.Close does not need the connection Feb 18, 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
2 participants
You can’t perform that action at this time.