Added options for configuring removal of idle connections #674
Conversation
pool.go
Outdated
| @@ -50,6 +60,11 @@ func (p *pool) Store(v wire) { | |||
| p.cond.L.Lock() | |||
| if !p.down && v.Error() == nil { | |||
| p.list = append(p.list, v) | |||
|
|
|||
| if p.idleConnTTL != nil { | |||
| p.removeIdleConns() | |||
There was a problem hiding this comment.
Hi @veirfuna,
I guess calling .removeIdleConns() alone in the .Store() can't remove idle connections. For example, if .Store() is not called, then idle connections are still there. You probably need to put .removeIdleConns() into a ticker.
and thanks for your contribution!
There was a problem hiding this comment.
Hi @rueian,
My first attempts were to create a goroutine that is observe idle connections and remove them. Do you see any problems in such solution? The only idea to not use goroutine was to add .removeIdleConns() to .Store()
There was a problem hiding this comment.
I think we have no choice but to use an additional goroutine. We can use time.AfterFunc and it is actually an additional goroutine. We can schedule the cleanup function only when idle connections > minIdle.
pool.go
Outdated
| stopChan chan struct{} | ||
| stopChanOnce *sync.Once |
There was a problem hiding this comment.
| stopChan chan struct{} | |
| stopChanOnce *sync.Once | |
| timer *time.Timer |
Can we just use time.AfterFunc and store the timer here? I think a timer is convenient for us because we can use .Stop() when len(p.list) <= minSize and .Reset() when len(p.list) > minSize.
pool.go
Outdated
| idleConnTTL *time.Duration | ||
| stopChan chan struct{} | ||
| stopChanOnce *sync.Once | ||
| lastUsage map[wire]time.Time |
There was a problem hiding this comment.
I am wondering if we need to keep track of idle time for each connection. Can we just clear every connection in the p.list that exceeds minSize whenever the timer is fired?
There was a problem hiding this comment.
Sound like a good idea. Made an update in PR
pool.go
Outdated
| if idleConnTTL != 0 { | ||
| p.timer = time.AfterFunc(idleConnTTL, p.removeIdleConns) | ||
| } |
There was a problem hiding this comment.
We don't need to start the timer at this point. There is no idle connection.
There was a problem hiding this comment.
Your idea is to start the timer only after len(p.list) > p.minSize?
pool.go
Outdated
| } | ||
|
|
||
| func (p *pool) resetTimerIfNeeded() { | ||
| if p.timer != nil && p.size > p.minSize { |
There was a problem hiding this comment.
| if p.timer != nil && p.size > p.minSize { | |
| if p.timer != nil && len(p.list) > p.minSize { |
p.size is all connections, including active ones. I think we only need to start the timer if idle connections len(p.list) exceed p.minSize.
pool.go
Outdated
| if p.idleConnTTL == 0 { | ||
| return | ||
| } |
There was a problem hiding this comment.
Can we remove this check here? I think we can do it when we create the timer.
| if p.idleConnTTL == 0 { | |
| return | |
| } |
pool.go
Outdated
| p.list = p.list[:newLen] | ||
| p.size = p.minSize | ||
|
|
||
| p.stopTimer() |
There was a problem hiding this comment.
| p.stopTimer() |
There was a problem hiding this comment.
Could you explain why you think the timer should not be stopped inside removeIdleConns?
There was a problem hiding this comment.
Isn't the timer already stopped at this point?
pool.go
Outdated
|
|
||
| func (p *pool) resetTimerIfNeeded() { | ||
| if p.timer != nil && p.size > p.minSize { | ||
| p.timer.Reset(p.idleConnTTL) |
There was a problem hiding this comment.
I just noticed that we can't always reset the timer. We can only reset it after it is fired, so we probably need another flag for that.
pool.go
Outdated
| return | ||
| } | ||
|
|
||
| newLen := len(p.list) - min(p.size-p.minSize, len(p.list)) |
There was a problem hiding this comment.
| newLen := len(p.list) - min(p.size-p.minSize, len(p.list)) | |
| newLen := min(p.minSize, len(p.list)) |
| p.cond.L.Lock() | ||
| defer p.cond.L.Unlock() | ||
|
|
||
| if p.down || p.size <= p.minSize || len(p.list) == 0 { |
There was a problem hiding this comment.
| if p.down || p.size <= p.minSize || len(p.list) == 0 { | |
| if p.down || len(p.list) <= p.minSize { |
| for _, w := range p.list[newLen:] { | ||
| w.Close() | ||
| } |
There was a problem hiding this comment.
| for _, w := range p.list[newLen:] { | |
| w.Close() | |
| } | |
| for i, w := range p.list[newLen:] { | |
| w.Close() | |
| p.list[i] = nil | |
| } |
There was a problem hiding this comment.
This allows those connections to be GC.
| } | ||
|
|
||
| p.list = p.list[:newLen] | ||
| p.size = p.minSize |
There was a problem hiding this comment.
| p.size = p.minSize |
Setting it directly to the p.minSize seems incorrect. We probably should put it in the above loop and use p.size-- instead.
| if p.timer == nil { | ||
| p.timer = time.AfterFunc(p.idleConnTTL, p.removeIdleConns) | ||
| return | ||
| } | ||
|
|
||
| p.timer.Reset(p.idleConnTTL) |
There was a problem hiding this comment.
| if p.timer == nil { | |
| p.timer = time.AfterFunc(p.idleConnTTL, p.removeIdleConns) | |
| return | |
| } | |
| p.timer.Reset(p.idleConnTTL) | |
| if p.timer == nil { | |
| p.timer = time.AfterFunc(p.idleConnTTL, p.removeIdleConns) | |
| } else { | |
| p.timer.Reset(p.idleConnTTL) | |
| } |
nit
Approach for #651