From bab077199cc12e2d3c37eb1a42db8f947165cf7b Mon Sep 17 00:00:00 2001 From: Seebs Date: Thu, 14 Nov 2019 17:00:34 -0600 Subject: [PATCH 1/2] use the right lock for Enqueue The request for a non-read lock blocks until all existing read locks exit, meaning that if an Immediate operation is already going for a fragment, an Enqueue operation will hang forever holding the fragment's lock, while the Immediate operation has probably relinquished the fragment's lock to wait for the queue worker to process it. But the queue worker can't process it, because the incoming Enqueue still holds the fragment's lock. Solution: Don't block the Enqueue operation like that. It shouldn't coexist with things that actually change the sq channels, like Stop(), but it is fine for it to coexist with other queue operations. --- snapshotqueue.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/snapshotqueue.go b/snapshotqueue.go index 210c2b772..c7eb8cabb 100644 --- a/snapshotqueue.go +++ b/snapshotqueue.go @@ -200,8 +200,8 @@ func (sq *prioritySnapshotQueue) Enqueue(f *fragment) { if f.snapshotPending { return } - sq.mu.Lock() - defer sq.mu.Unlock() + sq.mu.RLock() + defer sq.mu.RUnlock() if sq.normal == nil { sq.logger.Printf("requested snapshot after snapshot queue was closed") return From f204b37760b79b9eaf39405bae7193dd60448ae6 Mon Sep 17 00:00:00 2001 From: Seebs Date: Thu, 14 Nov 2019 17:39:25 -0600 Subject: [PATCH 2/2] use atomics instead of locking for stats --- snapshotqueue.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/snapshotqueue.go b/snapshotqueue.go index c7eb8cabb..d355824e3 100644 --- a/snapshotqueue.go +++ b/snapshotqueue.go @@ -18,6 +18,7 @@ import ( "fmt" "os" "sync" + "sync/atomic" "time" "github.com/pilosa/pilosa/v2/logger" @@ -108,8 +109,8 @@ type prioritySnapshotQueue struct { mu sync.RWMutex scanWG, workerWG sync.WaitGroup stats struct { - enqueued int64 - skipped int64 + enqueued uint64 + skipped uint64 } } @@ -214,10 +215,10 @@ func (sq *prioritySnapshotQueue) Enqueue(f *fragment) { // try to enqueue snapshot select { case sq.normal <- snapshotRequest{frag: f, when: time.Now()}: - sq.stats.enqueued++ + atomic.AddUint64(&sq.stats.enqueued, 1) return default: - sq.stats.skipped++ + atomic.AddUint64(&sq.stats.skipped, 1) f.snapshotPending = false return }