Copilot commented on code in PR #3438:
URL: https://github.com/apache/dubbo-go/pull/3438#discussion_r3426254627
##########
remoting/exchange_client.go:
##########
@@ -82,19 +82,28 @@ func NewExchangeClient(url *common.URL, client Client,
connectTimeout time.Durat
}
func (cl *ExchangeClient) doInit(url *common.URL) error {
- if cl.init {
+ if cl.init.Load() {
return nil
}
+ // Use CompareAndSwap to ensure only one goroutine performs
initialization.
+ // If CAS fails, another goroutine already initialized; spin-wait until
init completes.
+ if !cl.init.CAS(false, true) {
+ // Another goroutine is initializing; wait for it to complete.
+ for !cl.init.Load() {
+ time.Sleep(10 * time.Millisecond)
+ }
+ return nil
+ }
+ // This goroutine won the CAS — perform the actual initialization.
if cl.client.Connect(url) != nil {
// retry for a while
time.Sleep(100 * time.Millisecond)
if cl.client.Connect(url) != nil {
logger.Errorf("[Remoting] failed to connect server,
url=%v", url.Location)
+ cl.init.Store(false) // reset on failure so future
calls can retry
return errors.New("Failed to connect server " +
url.Location)
}
}
- // FIXME atomic operation
- cl.init = true
return nil
}
Review Comment:
The CAS sets `init` to true *before* `Connect` succeeds, so concurrent
callers may observe `init==true` and return nil while initialization is still
in progress (or even if it later fails and resets). This can lead to callers
proceeding with an unconnected client and can also cause errors from the
initializing goroutine to be lost to concurrent callers. Use a separate
initialization state (e.g., 3-state atomic: uninitialized / initializing /
initialized) plus a wait mechanism, or replace this with `sync.Once` + an error
channel/mutex to ensure (1) other goroutines wait for completion and (2) they
see the same error result.
##########
remoting/exchange_client.go:
##########
@@ -82,19 +82,28 @@ func NewExchangeClient(url *common.URL, client Client,
connectTimeout time.Durat
}
func (cl *ExchangeClient) doInit(url *common.URL) error {
- if cl.init {
+ if cl.init.Load() {
return nil
}
+ // Use CompareAndSwap to ensure only one goroutine performs
initialization.
+ // If CAS fails, another goroutine already initialized; spin-wait until
init completes.
+ if !cl.init.CAS(false, true) {
+ // Another goroutine is initializing; wait for it to complete.
+ for !cl.init.Load() {
+ time.Sleep(10 * time.Millisecond)
+ }
Review Comment:
This introduces a fixed-interval sleep-based spin loop, which adds avoidable
latency and can be wasteful under contention. If you keep an atomic-state
approach, prefer a proper coordination primitive (e.g., a `sync.Cond`, channel
signaling completion, or `sync.Once` + blocking wait) so waiters sleep until
notified rather than polling.
##########
remoting/exchange_client_test.go:
##########
@@ -104,3 +104,47 @@ func TestExchangeClientIsAvailable(t *testing.T) {
m.mu.Unlock()
assert.False(t, ec.IsAvailable())
}
+
+func TestExchangeClientConcurrentDoInit(t *testing.T) {
+ // Verify that concurrent calls to doInit only result in a single
Connect call.
+ m := &mockClient{available: true}
+ ec := NewExchangeClient(testURL(), m, 5*time.Second, true)
+
+ var wg sync.WaitGroup
+ const goroutines = 20
+ wg.Add(goroutines)
+ for i := 0; i < goroutines; i++ {
+ go func() {
+ defer wg.Done()
+ // All goroutines trigger lazy init concurrently via
Request-like paths
+ if err := ec.doInit(testURL()); err != nil {
+ t.Errorf("doInit failed: %v", err)
+ }
+ }()
+ }
Review Comment:
Calling `t.Errorf` from multiple goroutines can be problematic and is
commonly flagged by race/lint tooling. Prefer collecting errors in a buffered
channel (or `atomic`/mutex-protected slice) and asserting/reporting from the
main test goroutine after `wg.Wait()` to avoid potential data races and to
ensure clean test output.
--
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]
