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: Strange Errors when Closing a Tx's Prepared Statement after Commit #4459

Closed
gopherbot opened this Issue Nov 28, 2012 · 16 comments

Comments

Projects
None yet
7 participants
@gopherbot

by johnkgallagher:

What steps will reproduce the problem?

This test function demonstrates the problem (note that this won't actually run on
play.golang.org because of the dependency on go-sqlite3):
http://play.golang.org/p/mKL22tAoxR

The code is wrong, but the errors it produces are very confusing and don't at all point
to the actual problem. On line 19, a transaction's prepared statement's Close() is
deferred. However, the transaction is committed before returning from the function,
which means the Close() happens after the Commit. Depending on the driver, this can
cause:

1. Silent lockup (observed with both go-sqlite3 and github.com/bmizerany/pq)
2. Very odd driver messages (pq reported nonsensical errors, like "expected 4
arguments, got 1" on a query that had 1 argument)
3. Memory corruption panic (observed with go-sqlite3 (cgo))

What is the expected output?

Nothing (success)

What do you see instead?

Sometimes success, more often either a lockup or "panic: runtime error: invalid
memory address or nil pointer dereference". Commenting out line 19 and uncommenting
line 29 fixes the problem.

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

6g

Which operating system are you using?

Mac OS X, Linux

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

1.0.3

Please provide any additional information below.

I'm not sure this is necessarily a bug in database/sql, since the documentation does say
that a prepared statement can't be used after a transaction is committed/rolled back.
However, it would be nicer if it coped with this more gracefully, as it seemed
"natural" (although clearly wrong) to defer the Stmt.Close(). Any of the
following would have been preferable (to me, at least) than the debugging I did:

1. Transaction closes its open statements inside Commit/Rollback
2. Stmt.Close() panics if it's called outside the transaction
3. Stmt.Close() does nothing if it's called outside the transaction
@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Nov 29, 2012

Member

Comment 1:

Owner changed to @bradfitz.

Status changed to Accepted.

Member

bradfitz commented Nov 29, 2012

Comment 1:

Owner changed to @bradfitz.

Status changed to Accepted.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Dec 10, 2012

Contributor

Comment 2:

Labels changed: added size-m.

Contributor

rsc commented Dec 10, 2012

Comment 2:

Labels changed: added size-m.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Dec 10, 2012

Contributor

Comment 3:

Labels changed: added suggested.

Contributor

rsc commented Dec 10, 2012

Comment 3:

Labels changed: added suggested.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Dec 30, 2012

Contributor

Comment 4:

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

Contributor

rsc commented Dec 30, 2012

Comment 4:

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

@gwenn

This comment has been minimized.

Show comment
Hide comment
@gwenn

gwenn Jan 12, 2013

Comment 5:

Hello,
For the sqlite driver, I've found where the issue comes from:
The prepared statements are closed twice:
 - when the Tx is committed, the underlying conn is closed and the sqlite driver tries to close dangling statements,
 - with the deferred Stmt.Close.
Just comment out the following block in the sqlite driver to do a quick and dirty fix:
func (c *SQLiteConn) Close() error {
    /*s := C.sqlite3_next_stmt(c.db, nil)
    for s != nil {
        C.sqlite3_finalize(s)
        s = C.sqlite3_next_stmt(c.db, s)
    }*/
    rv := C.sqlite3_close(c.db)
I will attempt to fix the driver.
Regards.

gwenn commented Jan 12, 2013

Comment 5:

Hello,
For the sqlite driver, I've found where the issue comes from:
The prepared statements are closed twice:
 - when the Tx is committed, the underlying conn is closed and the sqlite driver tries to close dangling statements,
 - with the deferred Stmt.Close.
Just comment out the following block in the sqlite driver to do a quick and dirty fix:
func (c *SQLiteConn) Close() error {
    /*s := C.sqlite3_next_stmt(c.db, nil)
    for s != nil {
        C.sqlite3_finalize(s)
        s = C.sqlite3_next_stmt(c.db, s)
    }*/
    rv := C.sqlite3_close(c.db)
I will attempt to fix the driver.
Regards.
@gwenn

This comment has been minimized.

Show comment
Hide comment
@gwenn

gwenn Jan 13, 2013

Comment 6:

Ok,
I've just submitted a patch to mattn that fix the issue on the driver side:
mattn/go-sqlite3#37
For postgresql, I don't have it installed.
If you ask me, I can install it and investigate...
Regards.

gwenn commented Jan 13, 2013

Comment 6:

Ok,
I've just submitted a patch to mattn that fix the issue on the driver side:
mattn/go-sqlite3#37
For postgresql, I don't have it installed.
If you ask me, I can install it and investigate...
Regards.
@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Mar 12, 2013

Contributor

Comment 7:

Labels changed: added go1.1maybe, removed go1.1.

Contributor

rsc commented Mar 12, 2013

Comment 7:

Labels changed: added go1.1maybe, removed go1.1.

@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike May 18, 2013

Contributor

Comment 8:

Labels changed: added go1.2maybe, removed go1.1maybe.

Contributor

robpike commented May 18, 2013

Comment 8:

Labels changed: added go1.2maybe, removed go1.1maybe.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Sep 9, 2013

Contributor

Comment 9:

Labels changed: added go1.3maybe, removed go1.2maybe.

Contributor

rsc commented Sep 9, 2013

Comment 9:

Labels changed: added go1.3maybe, removed go1.2maybe.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Dec 4, 2013

Contributor

Comment 10:

Labels changed: added release-none, removed go1.3maybe.

Contributor

rsc commented Dec 4, 2013

Comment 10:

Labels changed: added release-none, removed go1.3maybe.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Dec 4, 2013

Contributor

Comment 11:

Labels changed: added repo-main.

Contributor

rsc commented Dec 4, 2013

Comment 11:

Labels changed: added repo-main.

@gopherbot

This comment has been minimized.

Show comment
Hide comment

Comment 12 by marko@joh.to:

This should be fixed in 1.4 by https://code.google.com/p/go/source/detail?r=50ce4ec65c4a.

@rsc rsc added this to the Unplanned milestone Apr 10, 2015

@s7v7nislands

This comment has been minimized.

Show comment
Hide comment

@bradfitz bradfitz removed their assignment Nov 8, 2016

@bradfitz

This comment has been minimized.

Show comment
Hide comment
Member

bradfitz commented Nov 8, 2016

@kardianos

This comment has been minimized.

Show comment
Hide comment
@kardianos

kardianos Nov 8, 2016

Contributor

I'll look into it.

Contributor

kardianos commented Nov 8, 2016

I'll look into it.

@kardianos

This comment has been minimized.

Show comment
Hide comment
@kardianos

kardianos Nov 16, 2016

Contributor

Yes, this could be closed.

Contributor

kardianos commented Nov 16, 2016

Yes, this could be closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.