Skip to content

Commit

Permalink
cache: fix a few minor goroutine leaks in leaf certs and the agent ca…
Browse files Browse the repository at this point in the history
…che (#17636)
  • Loading branch information
rboyer committed Jun 9, 2023
1 parent ca3b07d commit 92ee21b
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 9 deletions.
3 changes: 3 additions & 0 deletions .changelog/17636.txt
@@ -0,0 +1,3 @@
```release-note:bug
cache: fix a few minor goroutine leaks in leaf certs and the agent cache
```
20 changes: 13 additions & 7 deletions agent/cache-types/connect_ca_leaf.go
Expand Up @@ -12,12 +12,11 @@ import (
"github.com/mitchellh/hashstructure"

"github.com/hashicorp/consul/acl"
"github.com/hashicorp/consul/lib"

"github.com/hashicorp/consul/agent/cache"
"github.com/hashicorp/consul/agent/connect"
"github.com/hashicorp/consul/agent/consul"
"github.com/hashicorp/consul/agent/structs"
"github.com/hashicorp/consul/lib"
)

// Recommended name for registration.
Expand Down Expand Up @@ -426,20 +425,25 @@ func (c *ConnectCALeaf) Fetch(opts cache.FetchOptions, req cache.Request) (cache

// Setup the timeout chan outside the loop so we don't keep bumping the timeout
// later if we loop around.
timeoutCh := time.After(opts.Timeout)
timeoutTimer := time.NewTimer(opts.Timeout)
defer timeoutTimer.Stop()

// Setup initial expiry chan. We may change this if root update occurs in the
// loop below.
expiresCh := time.After(expiresAt.Sub(now))
expiresTimer := time.NewTimer(expiresAt.Sub(now))
defer func() {
// Resolve the timer reference at defer time, so we use the latest one each time.
expiresTimer.Stop()
}()

// Current cert is valid so just wait until it expires or we time out.
for {
select {
case <-timeoutCh:
case <-timeoutTimer.C:
// We timed out the request with same cert.
return lastResultWithNewState(), nil

case <-expiresCh:
case <-expiresTimer.C:
// Cert expired or was force-expired by a root change.
return c.generateNewLeaf(reqReal, lastResultWithNewState())

Expand Down Expand Up @@ -480,7 +484,9 @@ func (c *ConnectCALeaf) Fetch(opts cache.FetchOptions, req cache.Request) (cache
// loop back around, we'll wait at most delay until generating a new cert.
if state.forceExpireAfter.Before(expiresAt) {
expiresAt = state.forceExpireAfter
expiresCh = time.After(delay)
// Stop the former one and create a new one.
expiresTimer.Stop()
expiresTimer = time.NewTimer(delay)
}
continue
}
Expand Down
7 changes: 5 additions & 2 deletions agent/cache/cache.go
Expand Up @@ -71,7 +71,7 @@ const (
// rate limiter settings.

// DefaultEntryFetchRate is the default rate at which cache entries can
// be fetch. This defaults to not being unlimited
// be fetch. This defaults to not being limited
DefaultEntryFetchRate = rate.Inf

// DefaultEntryFetchMaxBurst is the number of cache entry fetches that can
Expand Down Expand Up @@ -510,7 +510,10 @@ RETRY_GET:

// Set our timeout channel if we must
if r.Info.Timeout > 0 && timeoutCh == nil {
timeoutCh = time.After(r.Info.Timeout)
timeoutTimer := time.NewTimer(r.Info.Timeout)
defer timeoutTimer.Stop()

timeoutCh = timeoutTimer.C
}

// At this point, we know we either don't have a value at all or the
Expand Down

0 comments on commit 92ee21b

Please sign in to comment.