Copilot commented on code in PR #3184:
URL: https://github.com/apache/dubbo-go/pull/3184#discussion_r2744367952
##########
protocol/base/base_invoker.go:
##########
@@ -36,6 +36,9 @@ import (
)
var (
+ // emptyURL is a global placeholder to prevent nil pointer dereferences
during invoker destruction.
+ // It is returned by GetURL() when the invoker is destroyed, ensuring
that concurrent readers receive a safe, initialized object.
+ emptyURL =
common.NewURLWithOptions(common.WithProtocol("empty"))
Review Comment:
Use the existing `constant.EmptyProtocol` instead of the hard-coded string
"empty" to avoid duplicating protocol literals (see
`common/constant/default.go:29`).
##########
protocol/base/base_invoker_test.go:
##########
@@ -72,7 +72,10 @@ func TestBaseInvokerWithFullURL(t *testing.T) {
ivk.Destroy()
assert.False(t, ivk.IsAvailable())
assert.True(t, ivk.IsDestroyed())
- assert.Nil(t, ivk.GetURL())
+
+ safeUrl := ivk.GetURL()
+ assert.NotNil(t, safeUrl)
+ assert.Equal(t, "empty", safeUrl.Protocol)
Review Comment:
Variable name `safeUrl` should use Go initialism casing (`safeURL`) to match
existing naming in this test (e.g., `returnedURL`).
```suggestion
safeURL := ivk.GetURL()
assert.NotNil(t, safeURL)
assert.Equal(t, "empty", safeURL.Protocol)
```
##########
protocol/base/base_invoker_test.go:
##########
@@ -72,7 +72,10 @@ func TestBaseInvokerWithFullURL(t *testing.T) {
ivk.Destroy()
assert.False(t, ivk.IsAvailable())
assert.True(t, ivk.IsDestroyed())
- assert.Nil(t, ivk.GetURL())
+
+ safeUrl := ivk.GetURL()
+ assert.NotNil(t, safeUrl)
+ assert.Equal(t, "empty", safeUrl.Protocol)
Review Comment:
The test hard-codes the protocol string "empty". Since the project already
defines `constant.EmptyProtocol`, using the constant here keeps tests aligned
with the rest of the codebase and avoids magic strings.
##########
protocol/base/base_invoker.go:
##########
@@ -71,6 +74,9 @@ func NewBaseInvoker(url *common.URL) *BaseInvoker {
// GetURL gets base invoker URL
func (bi *BaseInvoker) GetURL() *common.URL {
+ if bi.url == nil {
+ return emptyURL
+ }
return bi.url
Review Comment:
`bi.url` is read here while `Destroy()` writes to it without synchronization
(`bi.url = nil`). This remains a Go data race (and undefined behavior under the
memory model), even though the nil-deref panic is mitigated. Consider
protecting the URL pointer with `atomic.Pointer[*common.URL]` or a mutex, and
updating `Destroy()`/`GetURL()` accordingly.
##########
protocol/base/base_invoker.go:
##########
@@ -36,6 +36,9 @@ import (
)
var (
+ // emptyURL is a global placeholder to prevent nil pointer dereferences
during invoker destruction.
+ // It is returned by GetURL() when the invoker is destroyed, ensuring
that concurrent readers receive a safe, initialized object.
+ emptyURL =
common.NewURLWithOptions(common.WithProtocol("empty"))
Review Comment:
`emptyURL` is a package-level `*common.URL` returned to callers. Because
`common.URL` has publicly writable fields (e.g., `Ip`, `Port`, `Protocol`,
`Methods`) and callers sometimes mutate them (e.g., via Script Router),
returning a shared singleton can lead to cross-invoker state pollution and
potential data races if multiple goroutines touch the placeholder. Prefer
returning a per-invoker placeholder instance (allocate once during `Destroy`)
or another approach that avoids exposing a shared mutable `*URL`.
--
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]
