diff --git a/fs/march/march.go b/fs/march/march.go index effd240bd..ca1e554bf 100644 --- a/fs/march/march.go +++ b/fs/march/march.go @@ -263,14 +263,19 @@ func (m *March) Run(ctx context.Context) error { dstDepth: dstDepth - 1, noDst: m.NoCheckDest, } + // Discard the remaining jobs on cancellation to unblock the senders. + done := make(chan struct{}) go func() { - // when the context is cancelled discard the remaining jobs - <-m.Ctx.Done() - for range in { - traversing.Done() + select { + case <-m.Ctx.Done(): + for range in { + traversing.Done() + } + case <-done: } }() traversing.Wait() + close(done) close(in) wg.Wait() diff --git a/fs/march/march_test.go b/fs/march/march_test.go index 2a916c351..4d1ef745f 100644 --- a/fs/march/march_test.go +++ b/fs/march/march_test.go @@ -6,9 +6,11 @@ import ( "context" "errors" "fmt" + "runtime" "strings" "sync" "testing" + "time" _ "github.com/rclone/rclone/backend/local" "github.com/rclone/rclone/fs" @@ -378,6 +380,58 @@ func TestMarchNoProcessDstOnly(t *testing.T) { } } +// TestMarchNoGoroutineLeak checks that a march which completes normally (its +// context is never cancelled, as with an async rc job) does not strand the +// janitor goroutine that drains the job channel (See #9620) +func TestMarchNoGoroutineLeak(t *testing.T) { + r := fstest.NewRun(t) + + ctx := context.Background() + ctx, _ = fs.AddConfig(ctx) + // Write to both src and dst so the march lists real directories on both + // sides and completes cleanly (no missing-directory listing errors). + r.WriteBoth(ctx, "test", "hello world", t1) + r.WriteBoth(ctx, "sub dir/test2", "hello world", t1) + + runMarch := func() { + mt := &marchTester{ctx: ctx, cancel: func() {}} + m := &March{ + Ctx: ctx, + Fdst: r.Fremote, + Fsrc: r.Flocal, + Dir: "", + Callback: mt, + } + require.NoError(t, m.Run(ctx)) + } + + // Warm up so any one-time background goroutines are already started. + runMarch() + waitForGoroutines := func() int { + // Give any exiting goroutines a moment to unwind before counting. + var n int + for range 20 { + runtime.GC() + n = runtime.NumGoroutine() + time.Sleep(10 * time.Millisecond) + } + return n + } + before := waitForGoroutines() + + const runs = 20 + for range runs { + runMarch() + } + after := waitForGoroutines() + + // The leak stranded one goroutine per run. Allow a small slack for unrelated + // runtime goroutines, but well under the number of runs. + if after-before >= runs { + t.Fatalf("goroutine leak: %d before, %d after %d marches (delta %d)", before, after, runs, after-before) + } +} + // matchPair is a matched pair of direntries returned by matchListings type matchPair struct { src, dst fs.DirEntry