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

Jump to bottom

database/sql: SetConnMaxIdleTime without SetConnMaxLifetime has no effect #41114

Open
jeremyfaller opened this issue Aug 28, 2020 · 9 comments
Open

database/sql: SetConnMaxIdleTime without SetConnMaxLifetime has no effect #41114

jeremyfaller opened this issue Aug 28, 2020 · 9 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@jeremyfaller
Copy link
Contributor

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 commented Aug 29, 2020

/cc @kardianos

@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 30, 2020
@cagedmantis cagedmantis added this to the Backlog milestone Aug 30, 2020
@jeremyfaller
Copy link
Contributor Author

This is very likely fixed with:

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

@rsc
Copy link
Contributor

rsc commented Sep 18, 2020

@jeremyfaller Should we close this?

@sethvargo
Copy link
Contributor

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

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

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

@Warashi
Copy link
Contributor

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

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.

@agis
Copy link

agis commented May 24, 2022

@jeremyfaller So this can be closed, if I understand correctly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
7 participants
@rsc @cagedmantis @sethvargo @agis @jeremyfaller @Warashi @gopherbot