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.Statement may not be close #42205

Closed
coderYPG opened this issue Oct 26, 2020 · 6 comments
Closed

database/sql: driver.Statement may not be close #42205

coderYPG opened this issue Oct 26, 2020 · 6 comments
Milestone

Comments

@coderYPG
Copy link

@coderYPG coderYPG commented Oct 26, 2020

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

go version go1.15 darwin/amd64

Does this issue reproduce with the latest release?

yes

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

go env Output
GO111MODULE="on"
GOARCH="amd64"
GOBIN="/Users/lynn/go/bin"
GOCACHE="/Users/lynn/Library/Caches/go-build"
GOFLAGS=""
GOSUMDB="sum.golang.google.cn"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
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?

modify mysqlConn.PrepareContext

func (mc *mysqlConn) PrepareContext(ctx context.Context, query string) (driver.Stmt, error) {
    fmt.Println("stmt Created!")
    if err := mc.watchCancel(ctx); err != nil {
		return nil, err
	}
	...

and mysqlStmt.Close

func (stmt *mysqlStmt) Close() error {
    fmt.Println("stmt Close!") 
    if stmt.mc == nil || stmt.mc.closed.IsSet() {
	    return driver.ErrBadConn
    }
    ...

and run code below

func TestStatementNotClosed(t *testing.T) {
	db, err := sql.Open("mysql", "root:123@/db?charset=utf8&parseTime=True&loc=Local")
	if err != nil {
		t.Fatal(err)
	}

	ctx := context.Background()

	stmt, err := db.PrepareContext(ctx, "select field from  xxx where key = ?")
	
	// simulate others take a free connection
	conn, err := db.Conn(ctx)
	_ = conn
	
	if err != nil {
		t.Fatal(err)
	}
	defer stmt.Close()

	rows, err := stmt.QueryContext(ctx, "val")
	if err != nil {
		t.Fatal(err)
	}
	defer rows.Close()
	
	for rows.Next() {
		err := rows.Scan(&field)
		if err != nil {
			t.Fatal(err)
		}
		fmt.Printf("field=%+v",field)
	}

	err = rows.Err()
	if err != nil {
		t.Fatal(err)
	}

	return
}

What did you expect to see?

stmt Created!  
stmt Created!  
stmt Close!  
stmt Close!

What did you see instead?

stmt Created!  
stmt Created!  
stmt Close!  
@ianlancetaylor ianlancetaylor changed the title driver.Statement may not be close database/sql: driver.Statement may not be close Oct 26, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 26, 2020

@kardianos
Copy link
Contributor

@kardianos kardianos commented Oct 27, 2020

@coderYPG I'm not seeing why two stmts were created. Can you also print the SQL associated with each statement on fmt.Println("stmt Created!", query)? That may shed some light on this.

@coderYPG
Copy link
Author

@coderYPG coderYPG commented Oct 27, 2020

@coderYPG I'm not seeing why two stmts were created. Can you also print the SQL associated with each statement on fmt.Println("stmt Created!", query)? That may shed some light on this.

    I think i can explain it. DB.prepareDC prepare a driverStmt with a connection, and release(line 1503) the connection immediately.
    When statement exec, it first take a free connection from free list(line 1494). If the connection is not the previous connection the driverStmt prepared on, a new driverStmt is prepared on this connection and exec. The previous driverStmt will be closed by user with

defer stmt.Close()

but i don't see any call or hook to close the latter driverStmt.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Oct 27, 2020

What happens when you defer db.Close()?

statements should close after a time I believe, esp if you put time limits on open connections.

I'm not saying it is great, I don't like how prepared statements are designed in general, but I'm not sure if there is a bug here.

@coderYPG
Copy link
Author

@coderYPG coderYPG commented Oct 27, 2020

What happens when you defer db.Close()?

statements should close after a time I believe, esp if you put time limits on open connections.

I'm not saying it is great, I don't like how prepared statements are designed in general, but I'm not sure if there is a bug here.

    db.Close() not work.
    an opened statement is not closed, not close after a time.
    driver.Stmt.Close() will send a comStmtClose pkg to sql server to release the resource this stmt occupys.
So the implement nowaday may cause resource leaking.

I found tutorial has mention this problem

Because statements will be re-prepared as needed when their original connection is busy,  
it’s possible for high-concurrency usage of the database,  
which may keep a lot of connections busy, to create a large number of prepared statements.  
This can result in apparent leaks of statements, statements being prepared and re-prepared more often than you think,  
and even running into server-side limits on the number of statements.
@kardianos
Copy link
Contributor

@kardianos kardianos commented Oct 27, 2020

@coderYPG I can explain this:

conn 1 is opened when PrepareContext is ran.
conn 1 is then used when db.Conn() is called. It is not returned ever.
conn 2 is prepared and run when stmt.QueryContext is called, because conn 1 (which was orig prepared) is busy.
conn 2 is released back to the pool at the defer. However, the conn 1 stmt cannot be released until the connection returns.

In the above code, conn 1 is never returned.

This is working as intended. Below is code that can run in sql_test.go in the std library.


func TestStatementNotClosed(t *testing.T) {
	db := newTestDB(t, "people")
	defer closeDB(t, db)

	ctx := context.Background()

	stmt, err := db.PrepareContext(ctx, "SELECT|people|name|age=?")

	// simulate others take a free connection
	conn, err := db.Conn(ctx)
	const releaseConn = true
	if releaseConn {
		defer conn.Close()
	}

	if err != nil {
		t.Fatal(err)
	}
	defer stmt.Close()

	rows, err := stmt.QueryContext(ctx, 1)
	if err != nil {
		t.Fatal(err)
	}
	defer rows.Close()

	var name string
	for rows.Next() {
		err := rows.Scan(&name)
		if err != nil {
			t.Fatal(err)
		}
		fmt.Printf("name=%+v", name)
	}

	err = rows.Err()
	if err != nil {
		t.Fatal(err)
	}

}

If you switch releaseConn = false, then the test will fail because almost every unit test checks for pending connections. When the conn doesn't return, it raises a flag.

@kardianos kardianos closed this Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.