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: must call (*driver.Stmt).Close before correspondent (*driver.Conn).Close #5046

Closed
alexbrainman opened this issue Mar 14, 2013 · 6 comments
Assignees

Comments

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Mar 14, 2013

Recently, my code.google.com/p/odbc driver started crashing with "fatal error:
malloc/free - deadlock". It looks like memory corruption. There is plenty of room
for that in my package :-), but I cannot find any suspects. On the other hand, I can see
that my driver connection and correspondent statements are closed out of order -
connection first, followed by statement:

...
CALLERS: runtime.goexit /root/go/root/src/pkg/runtime/proc.c:1214
CALLERS: testing.tRunner /root/go/root/src/pkg/testing/testing.go:346
CALLERS: code.google.com/p/odbc_test.TestALEX
/root/go/path/mine/src/code.google.com/p/odbc/mssql_test.go:832
CALLERS: code.google.com/p/odbc_test.func·010
/root/go/path/mine/src/code.google.com/p/odbc/mssql_test.go:767
CALLERS: database/sql.(*DB).Close /root/go/root/src/pkg/database/sql/sql.go:284
CALLERS: code.google.com/p/odbc.(*Conn).Close
/root/go/path/mine/src/code.google.com/p/odbc/conn.go:45
CALLERS: code.google.com/p/odbc.releaseHandle
/root/go/path/mine/src/code.google.com/p/odbc/handle.go:36
releaseHandle: ht=2 h=822679440
SYSCALL_S: SQLFreeHandle(handleType=2, handle=822679440) ...
SYSCALL_E: SQLFreeHandle(handleType=2, handle=822679440) (ret=0)
releaseHandle(after): ret=0
CALLERS: runtime.goexit /root/go/root/src/pkg/runtime/proc.c:1214
CALLERS: code.google.com/p/odbc_test.func·011
/root/go/path/mine/src/code.google.com/p/odbc/mssql_test.go:795
CALLERS: database/sql.(*Stmt).Close /root/go/root/src/pkg/database/sql/sql.go:1031
CALLERS: database/sql.(*DB).removeDep /root/go/root/src/pkg/database/sql/sql.go:252
CALLERS: database/sql.(*Stmt).finalClose /root/go/root/src/pkg/database/sql/sql.go:1036
CALLERS: database/sql.(*DB).noteUnusedDriverStatement
/root/go/root/src/pkg/database/sql/sql.go:362
CALLERS: code.google.com/p/odbc.(*Stmt).Close
/root/go/path/mine/src/code.google.com/p/odbc/stmt.go:40
CALLERS: code.google.com/p/odbc.(*ODBCStmt).closeByStmt
/root/go/path/mine/src/code.google.com/p/odbc/odbcstmt.go:62
CALLERS: code.google.com/p/odbc.(*ODBCStmt).releaseHandle
/root/go/path/mine/src/code.google.com/p/odbc/odbcstmt.go:89
CALLERS: code.google.com/p/odbc.releaseHandle
/root/go/path/mine/src/code.google.com/p/odbc/handle.go:36
releaseHandle: ht=3 h=822679936
SYSCALL_S: SQLFreeHandle(handleType=3, handle=822679936) ...
fatal error: malloc/free - deadlock
[signal 0xc0000005 code=0x1 addr=0x2f0 pc=0x419cd8]
...

This is a no no (from http://goo.gl/O7mYb): "... After freeing the connection, it
is an application programming error to use the connection's handle in a call to an ODBC
function; doing so has undefined but probably fatal consequences ...". Clear enough
:-).

If database/sql package cannot provide such guarantee, perhaps, it needs to be
documented as such, so that all driver writers can take care of it in their code.

It is hard for me to provide simple test case - I need my database to demonstrate.

Alex
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 14, 2013

Comment 1:

I don't think we should document every guarantee we DON'T make.  We document the
guarantees we DO make.
You _can_ reproduce this in a test in the standard library with the fakeDB in
fakedb_test.go.  Just make it log calls to a public slice of events, and have a test
which clears that slice, does some actions (open + prepare + exec + close + whatever),
and then stringify the slice of events, looking for the right order.
Now that we have the dependency track stuff, this should be easy to "fix" (or start
guaranteeing) if you can write a stand-alone test.
You don't have much time, though.
@alexbrainman
Copy link
Member Author

@alexbrainman alexbrainman commented Mar 15, 2013

Comment 2:

Had to put panic in fakedb_test.go, because you ignore the error it returns.
diff -r 0c029965805f src/pkg/database/sql/fakedb_test.go
--- a/src/pkg/database/sql/fakedb_test.go   Thu Mar 14 15:01:45 2013 -0700
+++ b/src/pkg/database/sql/fakedb_test.go   Fri Mar 15 11:43:14 2013 +1100
@@ -237,7 +237,7 @@
        return errors.New("can't close fakeConn; already closed")
    }
    if c.stmtsMade > c.stmtsClosed {
-       return errors.New("can't close; dangling statement(s)")
+       panic(errors.New("can't close; dangling statement(s)"))
    }
    c.db = nil
    return nil
diff -r 0c029965805f src/pkg/database/sql/sql_test.go
--- a/src/pkg/database/sql/sql_test.go  Thu Mar 14 15:01:45 2013 -0700
+++ b/src/pkg/database/sql/sql_test.go  Fri Mar 15 11:43:14 2013 +1100
@@ -736,3 +736,61 @@
        t.Logf("stmt = %#v", stmt)
    }
 }
+
+func TestCloseConnBeforeStmts(t *testing.T) {
+   db := newTestDB(t, "people")
+   defer closeDB(t, db)
+
+   stmt, err := db.Prepare("SELECT|people|name|")
+   if err != nil {
+       t.Fatal(err)
+   }
+   defer stmt.Close()
+
+   eof := fmt.Errorf("eof")
+   query := func(ch chan error) (err2 error) {
+       defer func() {
+           ch <- err2
+       }()
+       ch <- nil // wait for other goroutines
+       r, err := stmt.Query()
+       if err != nil {
+           return fmt.Errorf("%v", err)
+       }
+       defer r.Close()
+       i := 0
+       ch <- nil // wait for other goroutines
+       for r.Next() {
+           ch <- nil // wait for other goroutines
+           var is string
+           err = r.Scan(&is)
+           if err != nil {
+               return fmt.Errorf("%v", err)
+           }
+           ch <- nil // wait for other goroutines
+           i++
+       }
+       err = r.Err()
+       if err != nil {
+           return fmt.Errorf("%v", err)
+       }
+       return eof
+   }
+
+   chs := make(map[int]chan error)
+   for i := 0; i < 3; i++ {
+       chs[i] = make(chan error)
+       go query(chs[i])
+   }
+   for len(chs) > 0 {
+       for i, ch := range chs {
+           err := <-ch
+           if err != nil {
+               if err != eof {
+                   t.Errorf("goroutine #%d failed: %v", i, err)
+               }
+               delete(chs, i)
+           }
+       }
+   }
+}
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 20, 2013

Comment 3:

That seems like an unnecessarily complex repro, but I see your point.  Could you save me
a minute or two and whittle it down to a few lines instead?
@alexbrainman
Copy link
Member Author

@alexbrainman alexbrainman commented Mar 21, 2013

Comment 4:

That is smallest I could do. Sorry.
Alex
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 25, 2013

Comment 5:

Mailed https://golang.org/cl/8016044

Status changed to Started.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 25, 2013

Comment 6:

This issue was closed by revision 209f6b1.

Status changed to Fixed.

@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.