radix.v2 icon indicating copy to clipboard operation
radix.v2 copied to clipboard

Initial sentinel dial has no timeout

Open fillest opened this issue 7 years ago • 2 comments

sentinel.NewClientCustom uses hardcoded redis.Dial at the beginning. Shouldn't it use the same passed df DialFunc instead? So one could pass a DialTimeout wrapper.

fillest avatar Aug 11 '17 09:08 fillest

Hi @fillest ! thanks for submitting the issue. The df DialFunc is defined as being for the connections being made to the redis master, not the sentinel instance itself. I'd like to keep it separated as people use the DialFunc for more than just timeouts (e.g. it can be used for AUTH commands as well), so the DialFunc for the actual redis instances might not be appropriate for the sentinel.

I've had a lot of issues submitted lately about the sentinel package though, it wasn't designed particularly well when I first did so, and I'm thinking I'll add a sentinel.NewClientWithOpts function which can take in an options struct (like in cluster.NewWithOpts) and use that to address a lot of the feedback. I'll definitely keep this issue in mind when I do so, and I'll reference this issue number in the commit so you'll get a notification.

mediocregopher avatar Aug 16 '17 15:08 mediocregopher

Hi. Ah, yeah, auth.. OK, thanks. In the meantime I'm wrapping it with:

func DoWithTimeout(cb func(), t time.Duration, panicValue interface{}) {
	done := make(chan bool, 1)
	go func() {
		defer someInternalPanicReporter()
		cb()
		done <- true
	}()
	select {
	case <-done:
	case <-time.After(t):
		panic(panicValue)
	}
}

...
	var err error
	helpers.DoWithTimeout(func() {
		client, err = sentinel.NewClientCustom("tcp", addr, POOL_SIZE, dial, "smth")
	}, 3*time.Second, "Redis Sentinel communication timeout")
	if err != nil {
		panic(err)
	}

fillest avatar Aug 16 '17 17:08 fillest