Fix deadlock when Stop and flush race

zapcore/buffered_write_syncer.go

@@ -188,32 +188,33 @@ func (s *BufferedWriteSyncer) flushLoop() {
 // Stop closes the buffer, cleans up background goroutines, and flushes
 // remaining unwritten data.
 func (s *BufferedWriteSyncer) Stop() (err error) {
-	var stopped bool
-
 	// Critical section.
-	func() {
+	stopped := func() bool {
 		s.mu.Lock()
 		defer s.mu.Unlock()
 
 		if !s.initialized {
-			return
+			return false
 		}
 
-		stopped = s.stopped
-		if stopped {
-			return
+		if s.stopped {
+			return false
 		}
 		s.stopped = true
 
 		s.ticker.Stop()
 		close(s.stop) // tell flushLoop to stop
-		<-s.done      // and wait until it has
+		return true
 	}()
 
-	// Don't call Sync on consecutive Stops.
+	// Not initialized, or already stopped, no need for any cleanup.
 	if !stopped {
-		err = s.Sync()
+		return
 	}
 
-	return err
+	// Wait for flushLoop to end outside of the lock, as it may need the lock to complete.
+	// See https://github.com/uber-go/zap/issues/1428 for details.
+	<-s.done
+
+	return s.Sync()
 }

zapcore/buffered_write_syncer_test.go

@@ -53,6 +53,14 @@ func TestBufferWriter(t *testing.T) {
 		assert.Equal(t, "foo", buf.String(), "Unexpected log string")
 	})
 
+	t.Run("stop race with flush", func(t *testing.T) {
+		buf := &bytes.Buffer{}
+		ws := &BufferedWriteSyncer{WS: AddSync(buf), FlushInterval: 1}
+		requireWriteWorks(t, ws)
+		assert.NoError(t, ws.Stop())
+		assert.Equal(t, "foo", buf.String(), "Unexpected log string")
+	})
+
 	t.Run("stop twice", func(t *testing.T) {
 		ws := &BufferedWriteSyncer{WS: &ztest.FailWriter{}}
 		_, err := ws.Write([]byte("foo"))