Skip to content

Commit

Permalink
Avoid panic on multiple closer.Signal calls (#1401)
Browse files Browse the repository at this point in the history
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.

Fixes DGRAPH-1581
  • Loading branch information
Ibrahim Jarif committed Oct 2, 2020
1 parent 4eeba5a commit e9e8f23
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 3 deletions.
10 changes: 7 additions & 3 deletions y/y.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -209,7 +210,10 @@ func (lc *Closer) AddRunning(delta int) {

// Signal signals the HasBeenClosed signal.
func (lc *Closer) Signal() {
close(lc.closed)
// Todo(ibrahim): Change Signal to return error on next badger breaking change.
lc.closeOnce.Do(func() {
close(lc.closed)
})
}

// HasBeenClosed gets signaled when Signal() is called.
Expand Down
17 changes: 17 additions & 0 deletions y/y_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() })
}

0 comments on commit e9e8f23

Please sign in to comment.