From ee59e61fddc6667538d529eec3b0b19c85d46f10 Mon Sep 17 00:00:00 2001 From: Ibrahim Jarif Date: Wed, 8 Jul 2020 18:49:57 +0530 Subject: [PATCH 1/2] Avoid panic on multiple closer.Signal calls The closer.Signal function closes a channel and multiple calls to the closing the same channel would cause a "channel already closed". This PR avoids the panic by using a sync.Once while closing the channel. Multiple calls to closer.Signal would be no-op with this commit. --- y/y.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/y/y.go b/y/y.go index 14122234f..ea857f1bd 100644 --- a/y/y.go +++ b/y/y.go @@ -191,8 +191,9 @@ func FixedDuration(d time.Duration) string { // to tell the goroutine to shut down, and a WaitGroup with which to wait for it to finish shutting // down. type Closer struct { - closed chan struct{} - waiting sync.WaitGroup + closed chan struct{} + waiting sync.WaitGroup + closeOnce sync.Once } // NewCloser constructs a new Closer, with an initial count on the WaitGroup. @@ -209,7 +210,9 @@ func (lc *Closer) AddRunning(delta int) { // Signal signals the HasBeenClosed signal. func (lc *Closer) Signal() { - close(lc.closed) + lc.closeOnce.Do(func() { + close(lc.closed) + }) } // HasBeenClosed gets signaled when Signal() is called. From 6eb1455d776a125b7d770887722eed67fca89b0f Mon Sep 17 00:00:00 2001 From: Ibrahim Jarif Date: Thu, 9 Jul 2020 14:46:31 +0530 Subject: [PATCH 2/2] Add todo and a unit test --- y/y.go | 1 + y/y_test.go | 17 +++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/y/y.go b/y/y.go index ea857f1bd..5e6dd5b25 100644 --- a/y/y.go +++ b/y/y.go @@ -210,6 +210,7 @@ func (lc *Closer) AddRunning(delta int) { // Signal signals the HasBeenClosed signal. func (lc *Closer) Signal() { + // Todo(ibrahim): Change Signal to return error on next badger breaking change. lc.closeOnce.Do(func() { close(lc.closed) }) diff --git a/y/y_test.go b/y/y_test.go index d1b963184..fe14cf4c8 100644 --- a/y/y_test.go +++ b/y/y_test.go @@ -254,3 +254,20 @@ func TestPagebufferReader4(t *testing.T) { require.Equal(t, err, io.EOF, "should return EOF") require.Equal(t, n, 0) } + +func TestMulipleSignals(t *testing.T) { + closer := NewCloser(0) + require.NotPanics(t, func() { closer.Signal() }) + // Should not panic. + require.NotPanics(t, func() { closer.Signal() }) + require.NotPanics(t, func() { closer.SignalAndWait() }) + + // Attempt 2. + closer = NewCloser(1) + require.NotPanics(t, func() { closer.Done() }) + + require.NotPanics(t, func() { closer.SignalAndWait() }) + // Should not panic. + require.NotPanics(t, func() { closer.SignalAndWait() }) + require.NotPanics(t, func() { closer.Signal() }) +}