Copilot commented on code in PR #3230:
URL: https://github.com/apache/dubbo-go/pull/3230#discussion_r2876086997
##########
filter/active/filter.go:
##########
@@ -63,22 +65,37 @@ func newActiveFilter() filter.Filter {
// Invoke starts to record the requests status
func (f *activeFilter) Invoke(ctx context.Context, invoker base.Invoker, inv
base.Invocation) result.Result {
- inv.(*invocation.RPCInvocation).SetAttachment(dubboInvokeStartTime,
strconv.FormatInt(base.CurrentTimeMillis(), 10))
- base.BeginCount(invoker.GetURL(), inv.MethodName())
+ rpcInv := inv.(*invocation.RPCInvocation)
+
+ // Record the start time
+ rpcInv.SetAttachment(dubboInvokeStartTime,
strconv.FormatInt(base.CurrentTimeMillis(), 10))
+
+ // Cache the URL object to ensure BeginCount and EndCount use the same
URL instance
+ // This prevents statistics inconsistency when the invoker is destroyed
concurrently
+ url := invoker.GetURL()
+ rpcInv.SetAttachment(dubboInvokeURL, url)
+
+ base.BeginCount(url, inv.MethodName())
return invoker.Invoke(ctx, inv)
}
// OnResponse update the active count base on the request result.
func (f *activeFilter) OnResponse(ctx context.Context, result result.Result,
invoker base.Invoker, inv base.Invocation) result.Result {
- startTime, err :=
strconv.ParseInt(inv.(*invocation.RPCInvocation).GetAttachmentWithDefaultValue(dubboInvokeStartTime,
"0"), 10, 64)
+ rpcInv := inv.(*invocation.RPCInvocation)
+
+ // Retrieve the cached URL from Invoke phase to ensure statistics
consistency
+ url := rpcInv.GetAttachmentInterface(dubboInvokeURL)
+
+ startTime, err :=
strconv.ParseInt(rpcInv.GetAttachmentWithDefaultValue(dubboInvokeStartTime,
"0"), 10, 64)
if err != nil {
result.SetError(err)
logger.Errorf("parse dubbo_invoke_start_time to int64 failed")
// When err is not nil, use default elapsed value of 1
- base.EndCount(invoker.GetURL(), inv.MethodName(), 1, false)
+ base.EndCount(url.(*common.URL), inv.MethodName(), 1, false)
return result
Review Comment:
`url := rpcInv.GetAttachmentInterface(dubboInvokeURL)` can return `nil` (or
a non-`*common.URL`), and the subsequent `url.(*common.URL)` will panic. Please
use a safe type assertion and fall back to a sensible default (e.g.,
`invoker.GetURL()` or skipping EndCount) if the cached value is missing, so
`OnResponse` stays robust even if the invocation didn’t pass through `Invoke`
or attachments were modified.
##########
filter/active/filter_test.go:
##########
@@ -52,20 +52,24 @@ func TestFilterInvoke(t *testing.T) {
invoker.EXPECT().GetURL().Return(url).Times(1)
filter.Invoke(context.Background(), invoker, invoc)
assert.NotEmpty(t,
invoc.GetAttachmentWithDefaultValue(dubboInvokeStartTime, ""))
+ urlFromAttachment, ok :=
invoc.GetAttachmentInterface(dubboInvokeURL).(*common.URL)
+ assert.True(t, ok, "dubboInvokeURL should be cached in attachment")
+ assert.Equal(t, url.Key(), urlFromAttachment.Key(), "Cached URL should
match original URL")
+
}
func TestFilterOnResponse(t *testing.T) {
c := base.CurrentTimeMillis()
elapsed := 100
+ url, _ :=
common.NewURL(fmt.Sprintf("dubbo://%s:%d/com.alibaba.user.UserProvider",
constant.LocalHostValue, constant.DefaultPort))
invoc := invocation.NewRPCInvocation("test", []any{"OK"},
map[string]any{
dubboInvokeStartTime: strconv.FormatInt(c-int64(elapsed), 10),
+ dubboInvokeURL: url,
})
Review Comment:
These tests currently place `dubboInvokeURL` into the invocation
attachments. If the implementation switches to using `Invocation` attributes
(recommended to avoid transporting internal state), update the tests to
set/read the cached URL via `SetAttribute`/`GetAttribute` instead so they
validate the intended behavior.
##########
filter/active/filter.go:
##########
@@ -63,22 +65,37 @@ func newActiveFilter() filter.Filter {
// Invoke starts to record the requests status
func (f *activeFilter) Invoke(ctx context.Context, invoker base.Invoker, inv
base.Invocation) result.Result {
- inv.(*invocation.RPCInvocation).SetAttachment(dubboInvokeStartTime,
strconv.FormatInt(base.CurrentTimeMillis(), 10))
- base.BeginCount(invoker.GetURL(), inv.MethodName())
+ rpcInv := inv.(*invocation.RPCInvocation)
+
+ // Record the start time
+ rpcInv.SetAttachment(dubboInvokeStartTime,
strconv.FormatInt(base.CurrentTimeMillis(), 10))
+
+ // Cache the URL object to ensure BeginCount and EndCount use the same
URL instance
+ // This prevents statistics inconsistency when the invoker is destroyed
concurrently
+ url := invoker.GetURL()
+ rpcInv.SetAttachment(dubboInvokeURL, url)
+
+ base.BeginCount(url, inv.MethodName())
Review Comment:
Storing `*common.URL` in `RPCInvocation` *attachments* will cause it to be
included in protocol payloads (e.g., Dubbo/Hessian RequestPayload uses
`invocation.Attachments()`), adding unnecessary network overhead and
potentially impacting cross-language interop. Since this is purely internal
state for the filter, store it in `Invocation` attributes instead (see
`Invocation` docs: attributes are not transported) and read it back via
`GetAttribute`/`GetAttributeWithDefaultValue`.
--
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]
