mirror of
https://github.com/rclone/rclone.git
synced 2026-07-09 23:35:06 -04:00
accounting: fix goroutine leak in NewStatsGroup for zero-transfer rc jobs
NewStatsGroup started the averageLoop goroutine unconditionally at group creation. In an rcd daemon driven by many short rc sync/move calls (a common pattern for scheduled spool flushes), each call gets a fresh job/N stats group. When such a job transferred zero files the loop was never stopped, because _stopAverageLoop is only reached via DoneTransferring once transferring and checking both go from non-empty back to empty, which never happens if nothing was ever transferring in the first place. The result was one leaked goroutine per rc call, growing unbounded until the daemon was OOM-killed (reported: ~61k goroutines and ~640 MB RSS after ~7 days from a per-minute timer over 6 mappings). This is the same class of leak as #8571, which fixed the equivalent auto-start in NewStats. Fix it the same way: do not start the average loop at group creation. NewTransfer and NewTransferRemoteSize already call startAverageLoop when real transfer activity begins, and DoneTransferring already stops it when the last transfer completes, so on-demand behaviour is unchanged for groups that actually do work. Groups that never transfer anything now cost zero goroutines. Adds a regression test that fails without the fix. Fixes #9567
This commit is contained in:
committed by
Nick Craig-Wood
parent
c1b5756eec
commit
c91c4cbbff
@@ -301,9 +301,16 @@ func GlobalStats() *StatsInfo {
|
||||
}
|
||||
|
||||
// NewStatsGroup creates new stats under named group.
|
||||
//
|
||||
// The averageLoop is intentionally NOT started here: it is started on
|
||||
// demand by NewTransfer / NewTransferRemoteSize when real transfer
|
||||
// activity begins and stopped by DoneTransferring when the last
|
||||
// transfer completes. Starting it eagerly leaked one goroutine per
|
||||
// group that never performed a transfer (issue #9567) - e.g. rcd
|
||||
// daemons driven by frequent rc sync/move calls where each call gets
|
||||
// a fresh job/N group and the source is often empty.
|
||||
func NewStatsGroup(ctx context.Context, group string) *StatsInfo {
|
||||
stats := NewStats(ctx)
|
||||
stats.startAverageLoop()
|
||||
stats.group = group
|
||||
groups.set(ctx, group, stats)
|
||||
return stats
|
||||
|
||||
@@ -271,3 +271,42 @@ func percentDiff(start, end uint64) uint64 {
|
||||
}
|
||||
return (diff * 100) / start
|
||||
}
|
||||
|
||||
// Regression for #9567: NewStatsGroup used to start the averageLoop
|
||||
// goroutine unconditionally at group creation. When an rcd daemon was
|
||||
// driven by many rc sync/move calls that transferred zero files, each
|
||||
// call created a fresh job/N group whose averageLoop was started but
|
||||
// never stopped (nothing ever went transferring->empty), leaking one
|
||||
// goroutine per call. The loop must only start on demand from
|
||||
// NewTransfer / NewTransferRemoteSize, and it must still stop when the
|
||||
// last transfer completes.
|
||||
func TestNewStatsGroupDoesNotStartAverageLoop(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
defer func() {
|
||||
groups = newStatsGroups()
|
||||
}()
|
||||
|
||||
stats := NewStatsGroup(ctx, "test-group-9567")
|
||||
|
||||
stats.mu.RLock()
|
||||
started := stats.average.started
|
||||
stats.mu.RUnlock()
|
||||
assert.False(t, started,
|
||||
"NewStatsGroup must not start averageLoop for a zero-transfer group (issue #9567)")
|
||||
|
||||
// The loop must still start on demand when a real transfer is added,
|
||||
// and stop cleanly once the last transfer completes.
|
||||
tr := stats.NewTransferRemoteSize("regression-9567", 0, nil, nil)
|
||||
stats.mu.RLock()
|
||||
startedAfterTransfer := stats.average.started
|
||||
stats.mu.RUnlock()
|
||||
assert.True(t, startedAfterTransfer,
|
||||
"averageLoop should start on demand when a transfer is added")
|
||||
|
||||
tr.Done(ctx, nil)
|
||||
stats.mu.RLock()
|
||||
startedAfterDone := stats.average.started
|
||||
stats.mu.RUnlock()
|
||||
assert.False(t, startedAfterDone,
|
||||
"averageLoop should stop when the last transfer is done")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user