Browse Source

fix: use defer cancel() with anonymous function for proper context cleanup

Fixed context cancellation to use defer pattern correctly in loop iteration.

Problem:
  for x := 0; x < n; x++ {
    timeoutCtx, cancel := context.WithTimeout(ctx, fc.grpcTimeout)
    err := pb.WithGrpcFilerClient(...)
    cancel() // Only called on normal return, not on panic
  }

Issues with original approach:
  1. If pb.WithGrpcFilerClient panics, cancel() is never called → context leak
  2. If callback returns early (though unlikely here), cleanup might be missed
  3. Not following Go best practices for context.WithTimeout usage

Problem with naive defer in loop:
  for x := 0; x < n; x++ {
    timeoutCtx, cancel := context.WithTimeout(ctx, fc.grpcTimeout)
    defer cancel() //  WRONG: All defers accumulate until function returns
  }

In Go, defer executes when the surrounding *function* returns, not when
the loop iteration ends. This would accumulate n deferred cancel() calls
and leak contexts until LookupVolumeIds returns.

Solution: Wrap in anonymous function
  for x := 0; x < n; x++ {
    err := func() error {
      timeoutCtx, cancel := context.WithTimeout(ctx, fc.grpcTimeout)
      defer cancel() //  Executes when anonymous function returns (per iteration)
      return pb.WithGrpcFilerClient(...)
    }()
  }

Benefits:
  ✓ Context always cancelled, even on panic
  ✓ defer executes after each iteration (not accumulated)
  ✓ Follows Go best practices for context.WithTimeout
  ✓ No resource leaks during retry loop execution
  ✓ Cleaner error handling

Reference:
  Go documentation for context.WithTimeout explicitly shows:
    ctx, cancel := context.WithTimeout(...)
    defer cancel()

This is the idiomatic pattern that should always be followed.
pull/7518/head
chrislu 2 weeks ago
parent
commit
bd8259c2a1
  1. 18
      weed/wdclient/filer_client.go

18
weed/wdclient/filer_client.go

@ -303,10 +303,14 @@ func (p *filerVolumeProvider) LookupVolumeIds(ctx context.Context, volumeIds []s
filerAddress := fc.filerAddresses[i]
// Create a fresh timeout context for each filer attempt
// This ensures each retry gets the full grpcTimeout, not a diminishing deadline
timeoutCtx, cancel := context.WithTimeout(ctx, fc.grpcTimeout)
err := pb.WithGrpcFilerClient(false, fc.clientId, filerAddress, fc.grpcDialOption, func(client filer_pb.SeaweedFilerClient) error {
// Use anonymous function to ensure defer cancel() is called per iteration, not accumulated
err := func() error {
// Create a fresh timeout context for each filer attempt
// This ensures each retry gets the full grpcTimeout, not a diminishing deadline
timeoutCtx, cancel := context.WithTimeout(ctx, fc.grpcTimeout)
defer cancel() // Always clean up context, even on panic or early return
return pb.WithGrpcFilerClient(false, fc.clientId, filerAddress, fc.grpcDialOption, func(client filer_pb.SeaweedFilerClient) error {
resp, err := client.LookupVolume(timeoutCtx, &filer_pb.LookupVolumeRequest{
VolumeIds: volumeIds,
})
@ -345,9 +349,9 @@ func (p *filerVolumeProvider) LookupVolumeIds(ctx context.Context, volumeIds []s
}
}
return nil
})
cancel() // Clean up timeout context immediately after call returns
return nil
})
}()
if err != nil {
glog.V(1).Infof("FilerClient: filer %s lookup failed (attempt %d/%d, retry %d/%d): %v", filerAddress, x+1, n, retry+1, maxRetries, err)

Loading…
Cancel
Save