Copilot commented on code in PR #3439:
URL: https://github.com/apache/dubbo-go/pull/3439#discussion_r3426258721
##########
cluster/cluster/base/cluster_invoker.go:
##########
@@ -39,7 +41,7 @@ type BaseClusterInvoker struct {
Directory directory.Directory
AvailableCheck bool
Destroyed *atomic.Bool
- StickyInvoker base.Invoker
+ stickyInvoker stdatomic.Value // stores base.Invoker
Review Comment:
`sync/atomic.Value` cannot store `nil` (it panics), so
`invoker.setStickyInvoker(nil)` will crash at runtime. Also, `atomic.Value`
requires all stored values have the exact same concrete dynamic type; storing
different `base.Invoker` implementations over time can panic as soon as a
different concrete type is stored. A robust fix is to store a single,
consistent wrapper type (e.g., a struct containing `base.Invoker`) so the
stored dynamic type never changes and clearing becomes storing a non-nil
wrapper whose inner invoker is `nil`.
##########
cluster/cluster/base/cluster_invoker.go:
##########
@@ -50,6 +52,20 @@ func NewBaseClusterInvoker(directory directory.Directory)
BaseClusterInvoker {
}
}
+// getStickyInvoker returns the current sticky invoker in a race-free manner.
+func (invoker *BaseClusterInvoker) getStickyInvoker() base.Invoker {
+ v := invoker.stickyInvoker.Load()
+ if v == nil {
+ return nil
+ }
+ return v.(base.Invoker)
+}
+
+// setStickyInvoker sets the sticky invoker in a race-free manner.
+func (invoker *BaseClusterInvoker) setStickyInvoker(invokerVal base.Invoker) {
+ invoker.stickyInvoker.Store(invokerVal)
+}
Review Comment:
`sync/atomic.Value` cannot store `nil` (it panics), so
`invoker.setStickyInvoker(nil)` will crash at runtime. Also, `atomic.Value`
requires all stored values have the exact same concrete dynamic type; storing
different `base.Invoker` implementations over time can panic as soon as a
different concrete type is stored. A robust fix is to store a single,
consistent wrapper type (e.g., a struct containing `base.Invoker`) so the
stored dynamic type never changes and clearing becomes storing a non-nil
wrapper whose inner invoker is `nil`.
##########
cluster/cluster/base/cluster_invoker.go:
##########
@@ -100,19 +116,21 @@ func (invoker *BaseClusterInvoker) DoSelect(lb
loadbalance.LoadBalance, invocati
// Get the service method sticky config if have
sticky = url.GetMethodParamBool(invocation.MethodName(),
constant.StickyKey, sticky)
- if invoker.StickyInvoker != nil && !isInvoked(invoker.StickyInvoker,
invokers) {
- invoker.StickyInvoker = nil
+ stickyInvoker := invoker.getStickyInvoker()
+ if stickyInvoker != nil && !isInvoked(stickyInvoker, invokers) {
+ invoker.setStickyInvoker(nil)
+ stickyInvoker = nil
}
if sticky && invoker.AvailableCheck &&
- invoker.StickyInvoker != nil &&
invoker.StickyInvoker.IsAvailable() &&
- (invoked == nil || !isInvoked(invoker.StickyInvoker, invoked)) {
- return invoker.StickyInvoker
+ stickyInvoker != nil && stickyInvoker.IsAvailable() &&
+ (invoked == nil || !isInvoked(stickyInvoker, invoked)) {
+ return stickyInvoker
}
selectedInvoker = invoker.doSelectInvoker(lb, invocation, invokers,
invoked)
if sticky {
- invoker.StickyInvoker = selectedInvoker
+ invoker.setStickyInvoker(selectedInvoker)
}
Review Comment:
`sync/atomic.Value` cannot store `nil` (it panics), so
`invoker.setStickyInvoker(nil)` will crash at runtime. Also, `atomic.Value`
requires all stored values have the exact same concrete dynamic type; storing
different `base.Invoker` implementations over time can panic as soon as a
different concrete type is stored. A robust fix is to store a single,
consistent wrapper type (e.g., a struct containing `base.Invoker`) so the
stored dynamic type never changes and clearing becomes storing a non-nil
wrapper whose inner invoker is `nil`.
##########
cluster/cluster/base/cluster_invoker.go:
##########
@@ -39,7 +41,7 @@ type BaseClusterInvoker struct {
Directory directory.Directory
AvailableCheck bool
Destroyed *atomic.Bool
- StickyInvoker base.Invoker
+ stickyInvoker stdatomic.Value // stores base.Invoker
Review Comment:
This change removes/renames the previously exported `StickyInvoker` field to
an unexported `stickyInvoker`, which is a breaking API change for any external
package that referenced `BaseClusterInvoker.StickyInvoker`. If
`BaseClusterInvoker` is part of a public API surface, consider preserving
backward compatibility (e.g., keep `StickyInvoker` as a deprecated field or
provide exported accessor methods and avoid removing exported fields in a
non-major change).
##########
cluster/cluster/base/cluster_invoker_test.go:
##########
@@ -73,3 +74,73 @@ func TestStickyNormalWhenError(t *testing.T) {
result1 := base.DoSelect(random.NewRandomLoadBalance(),
invocation.NewRPCInvocation(baseClusterInvokerMethodName, nil, nil), invokers,
invoked)
assert.NotEqual(t, result, result1)
}
+
+// TestStickyConcurrentDoSelect verifies that concurrent calls to DoSelect
+// with sticky enabled do not cause a data race on StickyInvoker.
+func TestStickyConcurrentDoSelect(t *testing.T) {
+ var invokers []protocolbase.Invoker
+ for i := 0; i < 10; i++ {
+ url, _ := common.NewURL(fmt.Sprintf(baseClusterInvokerFormat,
i))
+ url.SetParam("sticky", "true")
+ invokers = append(invokers, clusterpkg.NewMockInvoker(url, 1))
+ }
+ base := &BaseClusterInvoker{}
+ base.AvailableCheck = true
+
+ lb := random.NewRandomLoadBalance()
+ invocation1 :=
invocation.NewRPCInvocation(baseClusterInvokerMethodName, nil, nil)
+
+ const concurrency = 100
+ var wg sync.WaitGroup
+ wg.Add(concurrency)
+ for i := 0; i < concurrency; i++ {
+ go func() {
+ defer wg.Done()
+ invoked := make([]protocolbase.Invoker, 0)
+ result := base.DoSelect(lb, invocation1, invokers,
invoked)
+ assert.NotNil(t, result)
+ // Second call should return the same sticky invoker
+ result2 := base.DoSelect(lb, invocation1, invokers,
invoked)
+ assert.NotNil(t, result2)
+ }()
+ }
+ wg.Wait()
+}
+
+// TestStickyConcurrentIsAvailableAndDoSelect verifies that concurrent
+// IsAvailable and DoSelect calls do not cause a data race on StickyInvoker.
Review Comment:
The test name/comments claim it verifies concurrent `IsAvailable` and
`DoSelect`, but the goroutine calls `base.getStickyInvoker()` directly rather
than `base.IsAvailable()`. Either update the comment/name to match what’s
exercised, or adjust the test to actually call `IsAvailable()` (e.g., by
providing a non-nil `Directory` stub) so it matches the stated intent.
##########
cluster/cluster/base/cluster_invoker_test.go:
##########
@@ -73,3 +74,73 @@ func TestStickyNormalWhenError(t *testing.T) {
result1 := base.DoSelect(random.NewRandomLoadBalance(),
invocation.NewRPCInvocation(baseClusterInvokerMethodName, nil, nil), invokers,
invoked)
assert.NotEqual(t, result, result1)
}
+
+// TestStickyConcurrentDoSelect verifies that concurrent calls to DoSelect
+// with sticky enabled do not cause a data race on StickyInvoker.
+func TestStickyConcurrentDoSelect(t *testing.T) {
+ var invokers []protocolbase.Invoker
+ for i := 0; i < 10; i++ {
+ url, _ := common.NewURL(fmt.Sprintf(baseClusterInvokerFormat,
i))
+ url.SetParam("sticky", "true")
+ invokers = append(invokers, clusterpkg.NewMockInvoker(url, 1))
+ }
+ base := &BaseClusterInvoker{}
+ base.AvailableCheck = true
+
+ lb := random.NewRandomLoadBalance()
+ invocation1 :=
invocation.NewRPCInvocation(baseClusterInvokerMethodName, nil, nil)
+
+ const concurrency = 100
+ var wg sync.WaitGroup
+ wg.Add(concurrency)
+ for i := 0; i < concurrency; i++ {
+ go func() {
+ defer wg.Done()
+ invoked := make([]protocolbase.Invoker, 0)
+ result := base.DoSelect(lb, invocation1, invokers,
invoked)
+ assert.NotNil(t, result)
+ // Second call should return the same sticky invoker
+ result2 := base.DoSelect(lb, invocation1, invokers,
invoked)
+ assert.NotNil(t, result2)
Review Comment:
The test comment states the second call should return the same sticky
invoker, but the test doesn’t assert that invariant. Consider asserting `result
== result2` (or equivalent) so the test verifies sticky semantics in addition
to relying on `-race`.
##########
cluster/cluster/base/cluster_invoker.go:
##########
@@ -100,19 +116,21 @@ func (invoker *BaseClusterInvoker) DoSelect(lb
loadbalance.LoadBalance, invocati
// Get the service method sticky config if have
sticky = url.GetMethodParamBool(invocation.MethodName(),
constant.StickyKey, sticky)
- if invoker.StickyInvoker != nil && !isInvoked(invoker.StickyInvoker,
invokers) {
- invoker.StickyInvoker = nil
+ stickyInvoker := invoker.getStickyInvoker()
+ if stickyInvoker != nil && !isInvoked(stickyInvoker, invokers) {
+ invoker.setStickyInvoker(nil)
+ stickyInvoker = nil
}
Review Comment:
`sync/atomic.Value` cannot store `nil` (it panics), so
`invoker.setStickyInvoker(nil)` will crash at runtime. Also, `atomic.Value`
requires all stored values have the exact same concrete dynamic type; storing
different `base.Invoker` implementations over time can panic as soon as a
different concrete type is stored. A robust fix is to store a single,
consistent wrapper type (e.g., a struct containing `base.Invoker`) so the
stored dynamic type never changes and clearing becomes storing a non-nil
wrapper whose inner invoker is `nil`.
##########
cluster/cluster/base/cluster_invoker_test.go:
##########
@@ -73,3 +74,73 @@ func TestStickyNormalWhenError(t *testing.T) {
result1 := base.DoSelect(random.NewRandomLoadBalance(),
invocation.NewRPCInvocation(baseClusterInvokerMethodName, nil, nil), invokers,
invoked)
assert.NotEqual(t, result, result1)
}
+
+// TestStickyConcurrentDoSelect verifies that concurrent calls to DoSelect
+// with sticky enabled do not cause a data race on StickyInvoker.
+func TestStickyConcurrentDoSelect(t *testing.T) {
+ var invokers []protocolbase.Invoker
+ for i := 0; i < 10; i++ {
+ url, _ := common.NewURL(fmt.Sprintf(baseClusterInvokerFormat,
i))
+ url.SetParam("sticky", "true")
+ invokers = append(invokers, clusterpkg.NewMockInvoker(url, 1))
+ }
+ base := &BaseClusterInvoker{}
+ base.AvailableCheck = true
+
+ lb := random.NewRandomLoadBalance()
+ invocation1 :=
invocation.NewRPCInvocation(baseClusterInvokerMethodName, nil, nil)
+
+ const concurrency = 100
+ var wg sync.WaitGroup
+ wg.Add(concurrency)
+ for i := 0; i < concurrency; i++ {
+ go func() {
+ defer wg.Done()
+ invoked := make([]protocolbase.Invoker, 0)
+ result := base.DoSelect(lb, invocation1, invokers,
invoked)
+ assert.NotNil(t, result)
+ // Second call should return the same sticky invoker
+ result2 := base.DoSelect(lb, invocation1, invokers,
invoked)
+ assert.NotNil(t, result2)
+ }()
+ }
+ wg.Wait()
+}
+
+// TestStickyConcurrentIsAvailableAndDoSelect verifies that concurrent
+// IsAvailable and DoSelect calls do not cause a data race on StickyInvoker.
+// Only the sticky invoker path is exercised (not the Directory fallback),
+// since Directory is nil in this test setup.
+func TestStickyConcurrentIsAvailableAndDoSelect(t *testing.T) {
+ var invokers []protocolbase.Invoker
+ for i := 0; i < 10; i++ {
+ url, _ := common.NewURL(fmt.Sprintf(baseClusterInvokerFormat,
i))
+ url.SetParam("sticky", "true")
+ invokers = append(invokers, clusterpkg.NewMockInvoker(url, 1))
+ }
+ base := &BaseClusterInvoker{}
+ base.AvailableCheck = true
+
+ lb := random.NewRandomLoadBalance()
+ invocation1 :=
invocation.NewRPCInvocation(baseClusterInvokerMethodName, nil, nil)
+
+ // First DoSelect to set the sticky invoker so IsAvailable uses the
sticky path
+ invoked := make([]protocolbase.Invoker, 0)
+ base.DoSelect(lb, invocation1, invokers, invoked)
+
+ const concurrency = 100
+ var wg sync.WaitGroup
+ wg.Add(concurrency * 2)
+ for i := 0; i < concurrency; i++ {
+ go func() {
+ defer wg.Done()
+ // IsAvailable reads StickyInvoker; this would race
without atomic protection
+ _ = base.getStickyInvoker()
Review Comment:
The test name/comments claim it verifies concurrent `IsAvailable` and
`DoSelect`, but the goroutine calls `base.getStickyInvoker()` directly rather
than `base.IsAvailable()`. Either update the comment/name to match what’s
exercised, or adjust the test to actually call `IsAvailable()` (e.g., by
providing a non-nil `Directory` stub) so it matches the stated intent.
--
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]
