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: if there is a panic during Stmt execution, we cannot release resources #50953

Open
iv-menshenin opened this issue Feb 1, 2022 · 9 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@iv-menshenin
Copy link

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

$ go version
go version go1.17.6 linux/amd64

Does this issue reproduce with the latest release?

  • yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/devalio/.cache/go-build"
GOENV="/home/devalio/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/devalio/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/devalio/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go-1.17"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go-1.17/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17.6"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build4248269622=/tmp/go-build -gno-record-gcc-switches"

What did you do?

	tx, err := db.BeginTx(ctx, nil)
	if err != nil {
		return err
	}
	defer tx.Commit()

	stmt, err := tx.Prepare(...)
	if err != nil {
		return err
	}
	defer stmt.Close()

	_, err = stmt.ExecContext(ctx, ...)  // the error occurs if there is a panic in the process
	if err != nil {
		return err
	}

This is the kind of algorithm that leads to a complete hang-up:

  1. open the transaction
  2. Prepare statement
  3. Execute prepared statement
  4. If there is a panic inside the execution - resources will not be fully released and closing the statement or transaction will cause the goroutine to hang

What did you expect to see?

I expect the pending functions to perform correctly and the goroutine to continue to panic
I do not expect the whole routine to hang up permanently

What did you see instead?

resources will not be fully released and closing the statement or transaction will cause the goroutine to hang

@iv-menshenin
Copy link
Author

I know what's causing it and I'm dealing with it now

@iv-menshenin
Copy link
Author

take a look at this code from database/sql
if the execution of the resultFromStatement function panics, then releaseConn will never be executed

func (s *Stmt) ExecContext(ctx context.Context, args ...interface{}) (Result, error) {
	s.closemu.RLock()
	defer s.closemu.RUnlock()

	var res Result
	strategy := cachedOrNewConn
	for i := 0; i < maxBadConnRetries+1; i++ {
		if i == maxBadConnRetries {
			strategy = alwaysNewConn
		}
		dc, releaseConn, ds, err := s.connStmt(ctx, strategy)
		if err != nil {
			if err == driver.ErrBadConn {
				continue
			}
			return nil, err
		}

		res, err = resultFromStatement(ctx, dc.ci, ds, args...)     // PANIC HERE!
		releaseConn(err)      // NEVER EXECUTED
		if err != driver.ErrBadConn {
			return res, err
		}
	}
	return nil, driver.ErrBadConn
}

This can happen if we implement a Valuer interface where panic occurs
I've seen libraries that use the reflect package inaccurately before and create a panic at this point

@iv-menshenin
Copy link
Author

@gopherbot
Copy link

Change https://golang.org/cl/382214 mentions this issue: database/sql: Stmt improved for more panic-stability

@kardianos
Copy link
Contributor

Yes, Valuer must not panic:

type Valuer interface {
	// Value returns a driver Value.
	// Value must not panic.
	Value() (Value, error)
}

To date I've not wanted to fix this.
I'll see if I can look at your CL to see how invasive this change is.

@iv-menshenin
Copy link
Author

Yes, Valuer must not panic

it's not just about Valuer any panic in this thread will cause the gorutine to freeze
I recently sent a fix to this library: https://github.com/ClickHouse/clickhouse-go
it was panic that caused freezing and Valuer was not used there

@iv-menshenin
Copy link
Author

. . .
database/sql 
  sql.go:2595
    res, err = resultFromStatement(ctx, dc.ci, ds, args...)
  sql.go:2622
    resi, err := ctxDriverStmtExec(ctx, ds.si, dargs)
  ctxutil.go:65
    return siCtx.ExecContext(ctx, nvdargs)

further down the stack, the library that implements low-level operations - panics

This is the example I came across when I discovered this error

There was no use of Valuer

@kardianos
Copy link
Contributor

Fair enough. I've heard you. I'll see what we can do.

@toothrot toothrot added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 4, 2022
@toothrot toothrot added this to the Backlog milestone Feb 4, 2022
@iv-menshenin
Copy link
Author

I tried to reproduce in the test the call stack that leads to the error. To do this I had to implement a couple of driver interfaces. This shows that we are not protected against applications freezing if the driver that can panic when executing Exec or Query.

When goroutine is frozen, we can't complete the test, so I added a 15-second timeout

In any case, I corrected the tests today to make the problem I'm solving even clearer
https://golang.org/cl/382214

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants