From de57a9ef51e07bcfed383616827cd6aea1b08393 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Sun, 19 Nov 2017 16:13:40 -0800 Subject: [PATCH] Gives back the lock before writing to the expire channel. The lock isn't needed after we clean up the expire bin, and as seen in #3700 we can get into a deadlock waiting to place the expire index into the channel while holding this lock. Fixes #3700 --- agent/consul/state/tombstone_gc.go | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/agent/consul/state/tombstone_gc.go b/agent/consul/state/tombstone_gc.go index a05bd1b0e9..459321bda3 100644 --- a/agent/consul/state/tombstone_gc.go +++ b/agent/consul/state/tombstone_gc.go @@ -141,8 +141,9 @@ func (t *TombstoneGC) nextExpires() time.Time { return adj } -// expireTime is used to expire the entries at the given time. -func (t *TombstoneGC) expireTime(expires time.Time) { +// purgeBin gets the index for the given bin and then deletes the bin. If there +// is no bin then this will return 0 for the index, which is ok. +func (t *TombstoneGC) purgeBin(expires time.Time) uint64 { t.Lock() defer t.Unlock() @@ -152,8 +153,18 @@ func (t *TombstoneGC) expireTime(expires time.Time) { // is no work to do. exp, ok := t.expires[expires] if !ok { - return + return 0 } delete(t.expires, expires) - t.expireCh <- exp.maxIndex + return exp.maxIndex +} + +// expireTime is used to expire the entries at the given time. +func (t *TombstoneGC) expireTime(expires time.Time) { + // This is careful to take the lock only while we are fetching the index + // since the channel write might get blocked for reasons that could also + // need to hint GC (see #3700). + if index := t.purgeBin(expires); index > 0 { + t.expireCh <- index + } }