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.Commit() & tx.rollback() has unuseful closemu.lock & closemu.unlock #53730

Closed
debug-LiXiwen opened this issue Jul 7, 2022 · 2 comments

Comments

@debug-LiXiwen
Copy link

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

$ go version
go 1.18

Does this issue reproduce with the latest release?

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/a/Library/Caches/go-build"
GOENV="/Users/a/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/a/go/pkg/mod"
GOOS="darwin"
GOPATH="/Users/a/go"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.google.cn"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.17.11"
GCCGO="gccgo"

What did you do?

Please see this code:

// Commit commits the transaction.
func (tx *Tx) Commit() error {
	// Check context first to avoid transaction leak.
	// If put it behind tx.done CompareAndSwap statement, we can't ensure
	// the consistency between tx.done and the real COMMIT operation.
	select {
	default:
	case <-tx.ctx.Done():
		if atomic.LoadInt32(&tx.done) == 1 {
			return ErrTxDone
		}
		return tx.ctx.Err()
	}
	if !atomic.CompareAndSwapInt32(&tx.done, 0, 1) {
		return ErrTxDone
	}

	// Cancel the Tx to release any active R-closemu locks.
	// This is safe to do because tx.done has already transitioned
	// from 0 to 1. Hold the W-closemu lock prior to rollback
	// to ensure no other connection has an active query.
	tx.cancel()
	tx.closemu.Lock()
	tx.closemu.Unlock()

	var err error
	withLock(tx.dc, func() {
		err = tx.txi.Commit()
	})
	if !errors.Is(err, driver.ErrBadConn) {
		tx.closePrepared()
	}
	tx.close(err)
	return err
}

What did you expect to see?

what is the use of the lock? unlock immediately after lock.

What did you see instead?

Is it possible to delete the lock code?

	tx.closemu.Lock()
	tx.closemu.Unlock()
@ZekeLu
Copy link
Contributor

ZekeLu commented Jul 7, 2022

Because we only want to ensure there is no active query, and the tx is already marked as done, so no new queries can start. so just locking and unlocking (W) should be sufficent.

Quoted from https://golang.org/cl/250178 (by @kardianos)

@debug-LiXiwen
Copy link
Author

I see, Thank you!

@golang golang locked and limited conversation to collaborators Jul 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants