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

Closed
jbaum98 opened this issue Jul 11, 2018 · 6 comments
Closed

database/sql: Tx.Rollback hangs after panic in Valuer.Value #26332

jbaum98 opened this issue Jul 11, 2018 · 6 comments
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@jbaum98
Copy link

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 11, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Jul 11, 2018
@ianlancetaylor
Copy link
Contributor

CC @kardianos

@kardianos
Copy link
Contributor

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

@jbaum98
Copy link
Author

jbaum98 commented Jul 11, 2018

Can/should I submit a PR?

@gopherbot
Copy link

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

@kardianos
Copy link
Contributor

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.

@gopherbot
Copy link

Change https://golang.org/cl/170700 mentions this issue: database/sql/driver: document Valuer must not panic

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants