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: SetConnMaxIdleTime without SetConnMaxLifetime has no effect #41114

Open
jeremyfaller opened this issue Aug 28, 2020 · 8 comments
Open
Milestone

Comments

@jeremyfaller
Copy link
Contributor

@jeremyfaller jeremyfaller commented Aug 28, 2020

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

$ go version
go version devel +b5c8d88bba Tue Aug 25 12:10:40 2020 -0400 darwin/amd64

Custom version of go branch [dev.link] (1.16 as of about a week ago).

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="/Users/jfaller/go/bin"
GOCACHE="/Users/jfaller/Library/Caches/go-build"
GOENV="/Users/jfaller/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/jfaller/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/jfaller/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/jfaller/src/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/jfaller/src/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/jfaller/go/db_test/go.mod"
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/kt/9h7gfgy955lgbm_cys108l2c002pwh/T/go-build532706798=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Run the following:

package main

import (
	"database/sql"
	"fmt"
	"os"
	"strconv"
	"time"
)

func getVar(name string) int {
	val := os.Getenv(name)
	if len(val) == 0 {
		panic(fmt.Sprintf("error getting: %v", name))
	}
	v, err := strconv.Atoi(val)
	if err != nil {
		panic(fmt.Sprintf("error parsing %v %v", name, err))
	}
	return v
}

func main() {
	db, err := // connect to the db of your choice.
	if err != nil {
		panic(err)
	}
	defer db.Close()
	db.SetConnMaxIdleTime(time.Second * time.Duration(getVar("MAXIDLE")))
	db.SetConnMaxLifetime(time.Second * time.Duration(getVar("MAXLIFE")))
	db.SetMaxIdleConns(1)
	db.SetMaxOpenConns(1)
	sleep := time.Second*time.Duration(getVar("SLEEP"))
	for i := 0; i < 10; i++ {
		db.Ping()
		time.Sleep(sleep)
		print("\r", i)
	}
	fmt.Printf("\n%+v\n", db.Stats())
}

If you run the following with:

MAXIDLE=1 MAXLIFE=0 SLEEP=5 go run .

You expect to see some idle connections closed. If you set MAXLIFE as well, the program works correctly, eg:

MAXIDLE=1 MAXLIFE=2 SLEEP=5 go run .

What did you expect to see?

SetConnMaxIdleTime should have an effect without setting SetConnMaxLifetime

What did you see instead?

SetConnMaxIdleTime should either:

  • Have an effect without calling SetConnMaxLifetime
  • or be documented to have no effect without calling SetConnMaxLifetime

The error might be fixed by tweaking the callsite around shortestIdleTimeLocked

@rsc
Copy link
Contributor

@rsc rsc commented Aug 29, 2020

@cagedmantis cagedmantis added this to the Backlog milestone Aug 30, 2020
@jeremyfaller
Copy link
Contributor Author

@jeremyfaller jeremyfaller commented Aug 31, 2020

This is very likely fixed with:

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

@rsc
Copy link
Contributor

@rsc rsc commented Sep 18, 2020

@jeremyfaller Should we close this?

@sethvargo
Copy link
Contributor

@sethvargo sethvargo commented Sep 18, 2020

I don't think the logic in that CL is correct nor do I think it fixes this, but I could be misunderstanding.

After that CL, if maxIdle is less than 0, it is returned. I think the desire is for maxLifetime to be MAX(minIdleTimeout, maxLifetime).

@jeremyfaller
Copy link
Contributor Author

@jeremyfaller jeremyfaller commented Sep 18, 2020

I think Seth's correct. https://go-review.googlesource.com/c/go/+/248817/ will unintentionally fix some of the cases of this issue. I think that CL also doesn't completely address what it intended to address. I'll see about putting something together to fix this completely.

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 18, 2020

Change https://golang.org/cl/255966 mentions this issue: database/sql: add unit test confirming lifetimes

@Warashi
Copy link
Contributor

@Warashi Warashi commented Sep 23, 2020

Connection's idleTime has below constraints.

idleTime <= maxIdleTime && idleTime <= maxLifeTime

this can be rewrote as below.

idleTime <= min(maxIdleTime, maxLifeTime)

Connection's lifeTime also has below constraints.

lifeTime <= maxLifeTime

shortestIdleTimeLocked is used as a interval of connectionCleanerRunLocked.
connectionCleanerRunLocked should run every timing of expire can be occured, so interval should be min(maxIdleTime, maxLifeTime).

Am I misunderstunding?

@jeremyfaller
Copy link
Contributor Author

@jeremyfaller jeremyfaller commented Sep 23, 2020

I put together a CL that confirms the behavior through a test:

https://golang.org/cl/255966

Seth and I were both mistaken that there's a bug here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.