hanahmily commented on code in PR #1216:
URL:
https://github.com/apache/skywalking-banyandb/pull/1216#discussion_r3566247275
##########
banyand/internal/storage/disk_monitor.go:
##########
@@ -169,15 +173,24 @@ func (dm *DiskMonitor) Stop() {
}
close(dm.stopCh)
- // Wait for any active cleanup to finish
- for dm.isActive.Load() {
- time.Sleep(100 * time.Millisecond)
- }
+ // Wait for the monitor loop (including any in-flight cleanup cycle) to
+ // exit. The loop clears isActive on exit, so we must not poll isActive
+ // here: once stopCh is closed the loop may return without ever running
+ // another cleanup cycle, leaving isActive set and hanging Stop()
forever.
+ <-dm.doneCh
Review Comment:
Good catch — this is a real deadlock. Fixed in disk_monitor.go by adding a
`started atomic.Bool` field. Start() now sets `dm.started.Store(true)` at its
top (covering both the normal and the disabled/CheckInterval<=0 paths, both of
which guarantee doneCh is eventually closed). Stop() checks
`!dm.started.Load()` first and returns immediately when the monitor was never
started, so it no longer touches stopCh or blocks on doneCh in that case. I
chose an explicit `started` flag over gating on `dm.ticker != nil` because the
disabled path leaves ticker nil yet does close doneCh, so ticker alone doesn't
distinguish "never started" from "started-but-disabled".
##########
banyand/internal/storage/disk_monitor_test.go:
##########
@@ -244,6 +244,40 @@ func TestDiskMonitor_StartStop(t *testing.T) {
assert.False(t, dm.isActive.Load())
}
+func TestDiskMonitor_StopDoesNotHangWhenActive(t *testing.T) {
+ service := NewMockRetentionService()
+ config := RetentionConfig{
+ HighWatermark: 80.0,
+ LowWatermark: 60.0,
+ CheckInterval: time.Hour, // long interval so no cleanup
cycle races the test
+ Cooldown: time.Millisecond,
+ ForceCleanupEnabled: true,
+ }
+ registry := createMockMetricsRegistry()
+
+ dm := NewDiskMonitor(service, config, registry)
+ dm.Start()
+
+ // Simulate a forced cleanup that is still active when the monitor is
+ // stopped. Previously Stop() polled isActive, which the monitor loop
only
+ // clears from within a cleanup cycle; closing stopCh made the loop exit
+ // without clearing it, hanging Stop() forever.
+ dm.isActive.Store(true)
+
+ stopped := make(chan struct{})
+ go func() {
+ dm.Stop()
+ close(stopped)
+ }()
+
+ select {
+ case <-stopped:
+ case <-time.After(5 * time.Second):
+ t.Fatal("Stop() hung while a forced cleanup was active")
+ }
+ assert.False(t, dm.isActive.Load())
+}
Review Comment:
Agreed. Added TestDiskMonitor_StopBeforeStartDoesNotHang in
disk_monitor_test.go: it constructs a DiskMonitor and calls Stop() without ever
calling Start(), asserting via a goroutine + 5s timeout that Stop() returns
rather than hanging. Together with the existing
TestDiskMonitor_StopDoesNotHangWhenActive, this locks in both shutdown failure
modes so the deadlock can't be reintroduced.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
