Copilot commented on code in PR #3437:
URL: https://github.com/apache/dubbo-go/pull/3437#discussion_r3426151101
##########
remoting/codec_test.go:
##########
@@ -101,3 +102,28 @@ func TestCodecInterface(t *testing.T) {
assert.NotNil(t, result)
assert.Equal(t, 9, length)
}
+
+func TestConcurrentCodecAccess(t *testing.T) {
+ const goroutines = 100
+ var wg sync.WaitGroup
+ wg.Add(goroutines * 2)
+
+ // Concurrent writers
+ for i := 0; i < goroutines; i++ {
+ go func(i int) {
+ defer wg.Done()
+ RegistryCodec("concurrent-"+string(rune('a'+i%26)),
&mockCodec{})
+ }(i)
+ }
+
+ // Concurrent readers
+ for i := 0; i < goroutines; i++ {
+ go func(i int) {
+ defer wg.Done()
+ GetCodec("concurrent-" + string(rune('a'+i%26)))
+ }(i)
+ }
+
+ wg.Wait()
+ // If we reach here without a panic, the concurrent access is safe.
+}
Review Comment:
This test only checks for absence of a panic, but concurrent map access
issues and many race conditions won’t reliably surface as panics (they’re best
caught by the race detector). Consider (mandatory) updating the test to assert
something observable (e.g., that `GetCodec` returns non-nil for the expected
keys after writers complete) and (optional but strongly recommended) ensure CI
runs `go test -race` so data races are actually detected. Also, the comment at
line 128 is misleading as written; absence of panic does not prove safety.
##########
remoting/codec_test.go:
##########
@@ -101,3 +102,28 @@ func TestCodecInterface(t *testing.T) {
assert.NotNil(t, result)
assert.Equal(t, 9, length)
}
+
+func TestConcurrentCodecAccess(t *testing.T) {
+ const goroutines = 100
+ var wg sync.WaitGroup
+ wg.Add(goroutines * 2)
+
+ // Concurrent writers
+ for i := 0; i < goroutines; i++ {
+ go func(i int) {
+ defer wg.Done()
+ RegistryCodec("concurrent-"+string(rune('a'+i%26)),
&mockCodec{})
Review Comment:
This test mutates the package-level codec registry but doesn’t restore it.
That can make other tests order-dependent and harder to debug. Consider
capturing the current registry state at the start and restoring it via
`t.Cleanup(...)`, or clearing the inserted `concurrent-*` entries under the
same mutex once the test completes.
--
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]
