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: Tx.Rollback hangs after panic in Valuer.Value #26332

Open
jbaum98 opened this Issue Jul 11, 2018 · 5 comments

Comments

Projects
None yet
4 participants
@jbaum98

jbaum98 commented Jul 11, 2018

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

go1.10.2 darwin/amd64

Does this issue reproduce with the latest release?

Tested on go1.10.3 darwin/amd64 as well.

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/jake.waksbaum/Library/Caches/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/jake.waksbaum/go-src"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.10.3/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.10.3/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
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"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/q_/d4dp83cn1jb1t9yrx5x46mhr0000gn/T/go-build472845777=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Must have a PostgreSQL server setup locally with a database called test_bug.

package main

import (
	"database/sql"
	"database/sql/driver"
	"fmt"
	"os"

	_ "github.com/lib/pq"
)

var _ driver.Valuer = customData{}

type customData struct{}

func (customData) Value() (driver.Value, error) {
	panic("oh no!")
}

func main() {
	db, err := sql.Open("postgres", "dbname=test_bug sslmode=disable")
	if err != nil {
		fmt.Printf("opening database: %v\n", err)
		os.Exit(1)
	}
	defer db.Close()

	tx, err := db.Begin()
	if err != nil {
		fmt.Printf("beginning transaction: %v\n", err)
		os.Exit(1)
	}
	defer tx.Rollback()

	var flag bool
	err = tx.QueryRow("SELECT TRUE WHERE $1", customData{}).Scan(&flag)
	if err != nil {
		fmt.Printf("executing query: %v\n", err)
		os.Exit(1)
	}

	fmt.Printf("we got a %v\n", flag)
	os.Exit(0)
}

What did you expect to see?

Some sort of indication of a panic, followed by the program exiting.

What did you see instead?

Nothing. The program hangs with no output.

What I think is the problem

In sql.go in ExecContext, releaseConn is not deferred so when a panic happens in a Valuer.Value call deep within resultFromStatement, there is some lock being held on to. Then, when Tx.Rollback is called because it has been deferred, it hangs, stopping the unrolling of the stack and preventing any output from being shown or the process from exiting cleanly. I think we should change lines 2316-2320 to something like

res, err = func() (res Result, err error) {
	defer func() {
		p := recover()
		if p != nil {
			releaseConn(fmt.Errorf("panic: %v", p))
			panic(p)
		}
		releaseConn(err)
	}()

	return resultFromStatement(ctx, dc.ci, ds, args...)
}()

if err != driver.ErrBadConn {
	return res, err
}

@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Jul 11, 2018

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Jul 11, 2018

@kardianos

This comment has been minimized.

Contributor

kardianos commented Jul 11, 2018

I don't think this is a regression. We can consider a cl for 1.12.

@jbaum98

This comment has been minimized.

jbaum98 commented Jul 11, 2018

Can/should I submit a PR?

@gopherbot

This comment has been minimized.

gopherbot commented Sep 30, 2018

Change https://golang.org/cl/138579 mentions this issue: database/sql: add a test where the value panics

@kardianos

This comment has been minimized.

Contributor

kardianos commented Sep 30, 2018

I sent a CL that adds a test that demonstrates this behavior. This is to help me understand what is involved in this issue.

In the entire database/sql codebase, we don't use a single recover. As a matter of course, the package should gracefully handle locks and to let the user code continue panicing if the driver panics. I don't think database/sql should recover. I'm torn between adding documentation that the Valuer must not panic and trying to handle the panic without locks being in a bad state.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.12, Go1.13 Dec 11, 2018

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