fix: prevent send-on-closed-channel panic in concurrent Put/Delete + Close (C7)
Put/Delete checked writeStopped then called queue.Submit (which sends on a channel). Close set writeStopped then closed the channel. With no synchronization between the check and Submit, a concurrent Close could close the channel during the window, causing "send on closed channel" panic in Put/Delete. The CommitQueue.Close comment said "Callers must stop submitting before Close" but Put/Delete didn't enforce this. Changes: - wal/writer.go: add submitMu sync.RWMutex. Put/Delete construct entry outside the lock, then hold RLock during writeStopped check + Submit; release before <-req.Result. Close holds write lock during writeStopped.Store + queue.Close, guaranteeing no Submit is in progress when the channel is closed. - wal/writer_test.go: add TestConcurrentPutCloseNoPanic and TestConcurrentDeleteCloseNoPanic. 100 goroutines + 1 closer, recover panics. Run with -count=50 for regression detection. Verified: all existing tests pass. New concurrent tests pass with -count=50 (0 panics). go test -race ./... green. Audit context: docs/audit-3.2.md C7.
This commit is contained in:
+31
-12
@@ -58,6 +58,7 @@ type WalWriter struct {
|
||||
seqManager *SequenceManager
|
||||
memTables *MemTableList
|
||||
writeStopped atomic.Bool
|
||||
submitMu sync.RWMutex // C7 fix: protects Submit vs Close critical section
|
||||
done chan struct{}
|
||||
wg sync.WaitGroup
|
||||
closeOnce sync.Once
|
||||
@@ -311,31 +312,43 @@ func (ww *WalWriter) sendSuccess(requests []*CommitRequest, baseSequence uint64,
|
||||
|
||||
// Put stores key with value.
|
||||
func (ww *WalWriter) Put(key, value []byte) error {
|
||||
if ww.writeStopped.Load() {
|
||||
return errkit.ErrWriteStopped
|
||||
}
|
||||
|
||||
req := ww.queue.Submit([]*WalEntry{{
|
||||
entry := &WalEntry{
|
||||
OpType: OpPut,
|
||||
ValueKind: VKInline,
|
||||
Key: cloneBytes(key),
|
||||
Value: cloneBytes(value),
|
||||
}})
|
||||
}
|
||||
|
||||
// C7 fix: RLock protects check + Submit. Close holds write lock during
|
||||
// writeStopped.Store + queue.Close, so Submit cannot send on a closed channel.
|
||||
ww.submitMu.RLock()
|
||||
if ww.writeStopped.Load() {
|
||||
ww.submitMu.RUnlock()
|
||||
return errkit.ErrWriteStopped
|
||||
}
|
||||
req := ww.queue.Submit([]*WalEntry{entry})
|
||||
ww.submitMu.RUnlock()
|
||||
|
||||
result := <-req.Result
|
||||
return result.Err
|
||||
}
|
||||
|
||||
// Delete removes key.
|
||||
func (ww *WalWriter) Delete(key []byte) error {
|
||||
if ww.writeStopped.Load() {
|
||||
return errkit.ErrWriteStopped
|
||||
}
|
||||
|
||||
req := ww.queue.Submit([]*WalEntry{{
|
||||
entry := &WalEntry{
|
||||
OpType: OpDelete,
|
||||
ValueKind: VKNone,
|
||||
Key: cloneBytes(key),
|
||||
}})
|
||||
}
|
||||
|
||||
ww.submitMu.RLock()
|
||||
if ww.writeStopped.Load() {
|
||||
ww.submitMu.RUnlock()
|
||||
return errkit.ErrWriteStopped
|
||||
}
|
||||
req := ww.queue.Submit([]*WalEntry{entry})
|
||||
ww.submitMu.RUnlock()
|
||||
|
||||
result := <-req.Result
|
||||
return result.Err
|
||||
}
|
||||
@@ -368,8 +381,14 @@ func (ww *WalWriter) IsWriteStopped() bool {
|
||||
// Close drains queued writes, stops the writer goroutine, syncs, and closes the segment.
|
||||
func (ww *WalWriter) Close() error {
|
||||
ww.closeOnce.Do(func() {
|
||||
// C7 fix: write lock waits for all in-flight Put/Delete Submits to
|
||||
// complete. While we hold this lock, no Submit can be in progress,
|
||||
// so queue.Close is safe (no send-on-closed-channel panic).
|
||||
ww.submitMu.Lock()
|
||||
ww.writeStopped.Store(true)
|
||||
ww.queue.Close()
|
||||
ww.submitMu.Unlock()
|
||||
|
||||
close(ww.done)
|
||||
ww.wg.Wait()
|
||||
if err := ww.segManager.Sync(); err != nil {
|
||||
|
||||
@@ -4,6 +4,7 @@ import (
|
||||
"errors"
|
||||
"fmt"
|
||||
"sync"
|
||||
"sync/atomic"
|
||||
"testing"
|
||||
|
||||
"github.com/dailz/go-kv/config"
|
||||
@@ -164,3 +165,101 @@ func TestWriteStoppedAfterIOError(t *testing.T) {
|
||||
|
||||
_ = ww.Close()
|
||||
}
|
||||
|
||||
// -------- C7 regression guards --------
|
||||
|
||||
// Regression guard for C7: concurrent Put + Close must not panic.
|
||||
// Buggy code has a race window between writeStopped check and Submit's
|
||||
// send on channel; Close closing the channel during that window causes
|
||||
// "send on closed channel" panic.
|
||||
//
|
||||
// Note: this test guarantees "no panic AFTER fix", not "must panic BEFORE
|
||||
// fix" — the race window is narrow (Put check → Submit, a few instructions)
|
||||
// and the default queue capacity (10000) means sends rarely block. Run
|
||||
// with -count=50 to increase regression detection probability.
|
||||
func TestConcurrentPutCloseNoPanic(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
cfg := config.Defaults()
|
||||
ww, err := NewWalWriter(&cfg, dir, 0, 0)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
const goroutines = 100
|
||||
var wg sync.WaitGroup
|
||||
var panicCount atomic.Int32
|
||||
start := make(chan struct{})
|
||||
|
||||
for i := 0; i < goroutines; i++ {
|
||||
wg.Add(1)
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
panicCount.Add(1)
|
||||
}
|
||||
}()
|
||||
<-start
|
||||
_ = ww.Put([]byte("k"), []byte("v"))
|
||||
}()
|
||||
}
|
||||
|
||||
wg.Add(1)
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
<-start
|
||||
_ = ww.Close()
|
||||
}()
|
||||
|
||||
close(start)
|
||||
wg.Wait()
|
||||
|
||||
if panicCount.Load() > 0 {
|
||||
t.Fatalf("concurrent Put + Close caused %d panic(s) — C7 regression",
|
||||
panicCount.Load())
|
||||
}
|
||||
}
|
||||
|
||||
// Regression guard for C7: same as above but for Delete.
|
||||
func TestConcurrentDeleteCloseNoPanic(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
cfg := config.Defaults()
|
||||
ww, err := NewWalWriter(&cfg, dir, 0, 0)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
const goroutines = 100
|
||||
var wg sync.WaitGroup
|
||||
var panicCount atomic.Int32
|
||||
start := make(chan struct{})
|
||||
|
||||
for i := 0; i < goroutines; i++ {
|
||||
wg.Add(1)
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
panicCount.Add(1)
|
||||
}
|
||||
}()
|
||||
<-start
|
||||
_ = ww.Delete([]byte("k"))
|
||||
}()
|
||||
}
|
||||
|
||||
wg.Add(1)
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
<-start
|
||||
_ = ww.Close()
|
||||
}()
|
||||
|
||||
close(start)
|
||||
wg.Wait()
|
||||
|
||||
if panicCount.Load() > 0 {
|
||||
t.Fatalf("concurrent Delete + Close caused %d panic(s) — C7 regression",
|
||||
panicCount.Load())
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user