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.

Sign up for GitHub

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

Jump to bottom

database/sql: sync.Mutex unlocked when panic cause by context #54110

Closed
fishery86 opened this issue Jul 28, 2022 · 5 comments
Closed

database/sql: sync.Mutex unlocked when panic cause by context #54110

fishery86 opened this issue Jul 28, 2022 · 5 comments
Labels
FrozenDueToAge

Comments

@fishery86
Copy link

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

$ go1.17.2 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=""
GOCACHE=""
GOENV=""
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOOS="darwin"
GOPATH=""
GOROOT="/usr/local/Cellar/go/1.17.2/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.17.2/libexec/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.17.2"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

// with nil context.Context
tx, err := db.BeginTx(nil, nil)
if err != nil {
return err
}
defer tx.Commit()

goroutine 17183 [running]:
runtime/debug.Stack(0x1358240, 0xc0004e1aa0, 0x15129f9)
	/usr/local/go/src/runtime/debug/stack.go:24 +0x9f
....
panic(0x1342740, 0x1ef49d0)
	/usr/local/go/src/runtime/panic.go:965 +0x1b9
**************/context.(*MyContext).Done(0x0, 0xc000e6aae8)
	<autogenerated>:1 +0x2b
database/sql.(*DB).conn(0xc000367ad0, 0x16d2dc8, 0x0, 0x1, 0x13341c0, 0xc0004e1770, 0xc0006dc700)
	/usr/local/go/src/database/sql/sql.go:1205 +0xd7
database/sql.(*DB).begin(0xc000367ad0, 0x16d2dc8, 0x0, 0x0, 0xc000091b01, 0xc000091d40, 0xfa3e0b, 0xc0004baa50)
	/usr/local/go/src/database/sql/sql.go:1752 +0x4f
database/sql.(*DB).BeginTx(0xc000367ad0, 0x16d2dc8, 0x0, 0x0, 0xc000367ad0, 0x1, 0xc000e6abd0)
	/usr/local/go/src/database/sql/sql.go:1734 +0x8f

panic happend in conn and it makes locker not be unlocked

// conn returns a newly-opened or cached *driverConn.
func (db *DB) conn(ctx context.Context, strategy connReuseStrategy) (*driverConn, error) {
	db.mu.Lock()
	if db.closed {
		db.mu.Unlock()
		return nil, errDBClosed
	}
	// Check if the context is expired.
	select {
	default:
	case <-ctx.Done(): // panic here and db.mu will not be unlocked ,it causes conns hang-up
		db.mu.Unlock()
		return nil, ctx.Err()
	}
	lifetime := db.maxLifetime
    // .....
}




### What did you expect to see?

db.mu.Unlock() should be called
@andig
Copy link
Contributor

andig commented Jul 28, 2022 •
edited
Loading

Why or how should ctx.Done() panic? Are you sure that's what is happening?

@fishery86
Copy link
Author

ctx.Done() should not panic normally,but it may panic unexpected. And ctx may unexpectly be nil, it makes the same result.

@fishery86 fishery86 mentioned this issue Jul 28, 2022
Closed
@ianlancetaylor
Copy link
Member

Don't pass a nil context. Go code doesn't try to handle every conceivable panic. This program has a logic error which should be caught by ordinary testing, and there is no need for the standard library to handle this case. Sorry.

@ianlancetaylor ianlancetaylor closed this as not planned Won't fix, can't repro, duplicate, stale Jul 28, 2022
@fishery86
Copy link
Author

fishery86 commented Jul 29, 2022 •
edited
Loading

Don't pass a nil context. Go code doesn't try to handle every conceivable panic. This program has a logic error which should be caught by ordinary testing, and there is no need for the standard library to handle this case. Sorry.

quite agree that. and I think the code now could be fixed to avoid hang-up when panic happened. As the PR #54144 does,it does not handle the panic caused by context ,but it avoid locker be unlocked,and make it running on right way.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/420074 mentions this issue: database/sql: check context expire before lock

@golang golang locked and limited conversation to collaborators Jul 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

database/sql: check context expire before lock fishery86/go
4 participants
@andig @ianlancetaylor @gopherbot @fishery86