database/sql: if there is a panic during Stmt execution, we cannot release resources

[iv-menshenin]

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]

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

[iv-menshenin]

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]

https://go-review.googlesource.com/c/go/+/382214

[gopherbot]

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

[kardianos]

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]

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]

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

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

[iv-menshenin]

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