Copilot commented on code in PR #3436:
URL: https://github.com/apache/dubbo-go/pull/3436#discussion_r3426068792
##########
cluster/cluster/forking/cluster_invoker.go:
##########
@@ -72,16 +72,25 @@ func (invoker *forkingClusterInvoker) Invoke(ctx
context.Context, invocation pro
}
resultQ := queue.New(1)
+ forkCtx, cancel := context.WithCancel(ctx)
+ defer cancel() // cancel forkCtx when Invoke returns, signaling all
forked goroutines to stop
Review Comment:
Right now `forkCtx` is only cancelled when `Invoke` returns (via `defer
cancel()`), so forked `k.Invoke(...)` calls will keep running even after `Poll`
has already returned a result. This can waste resources and prolong in-flight
work, especially if providers honor context cancelation. Consider canceling
immediately after `resultQ.Poll(...)` completes (success or error). If you
still want a safety net, you can keep the deferred cancel but also call
`cancel()` right after `Poll` returns to stop remaining goroutines promptly.
##########
cluster/cluster/forking/cluster_invoker.go:
##########
@@ -72,16 +72,25 @@ func (invoker *forkingClusterInvoker) Invoke(ctx
context.Context, invocation pro
}
resultQ := queue.New(1)
Review Comment:
To ensure the queue is always disposed even if new early-returns get added
later, consider disposing via `defer resultQ.Dispose()` immediately after
creation, and then optionally keep an explicit dispose after `Poll` only if you
need deterministic timing. This reduces the risk of future leaks from added
return paths.
##########
cluster/cluster/forking/cluster_invoker.go:
##########
@@ -72,16 +72,25 @@ func (invoker *forkingClusterInvoker) Invoke(ctx
context.Context, invocation pro
}
resultQ := queue.New(1)
+ forkCtx, cancel := context.WithCancel(ctx)
+ defer cancel() // cancel forkCtx when Invoke returns, signaling all
forked goroutines to stop
+
for _, ivk := range selected {
go func(k protocolbase.Invoker) {
- result := k.Invoke(ctx, invocation)
+ result := k.Invoke(forkCtx, invocation)
if err := resultQ.Put(result); err != nil {
- logger.Errorf("[Cluster][Forking] resultQ put
failed with exception err=%v", err)
+ // ErrDisposed is expected when another
goroutine's result was already
+ // consumed and the queue disposed; log at
debug level to avoid noise.
+ logger.Debugf("[Cluster][Forking] resultQ put
failed, the queue is probably disposed: %v", err)
Review Comment:
The comment indicates only `ErrDisposed` is expected, but the code logs
*all* `Put` errors at debug level and assumes disposal. That can hide real
issues (e.g., unexpected queue failures) and make production troubleshooting
harder. Suggest explicitly checking for the disposed error (e.g.,
`errors.Is(err, queue.ErrDisposed)`) and only downgrading that case to debug;
log unexpected errors at error/warn level with accurate messaging.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
