Draft: Fix race condition during CSM message exchange #611
Conversation
WalkthroughAdds a configurable CSM exchange timeout and option, updates the TCP client to wait for a peer CSM (or time out) during connection setup, introduces a TCP signal-received handler, extends client config and options, and updates tests to use context-aware dialing. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant TCPClient as TCP Client
participant Peer as Peer Server
App->>TCPClient: configure WithCSMExchangeTimeout(t)
App->>TCPClient: call Dial / Client(...)
TCPClient->>Peer: establish TCP (TLS via DialContext if applicable)
alt CSMExchangeTimeout>0 and peer CSM enabled
Note right of TCPClient: wait for CSM signal or timeout
Peer-->>TCPClient: send CSM (codes.CSM)
TCPClient->>TCPClient: invoke TCPSignalReceivedHandler(CSM)
TCPClient->>App: return client.Conn, nil
else timeout before CSM
TCPClient->>App: report timeout error, close conn
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #611 +/- ##
==========================================
+ Coverage 73.23% 73.55% +0.32%
==========================================
Files 73 73
Lines 6986 7030 +44
==========================================
+ Hits 5116 5171 +55
+ Misses 1486 1480 -6
+ Partials 384 379 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tcp/client/conn.go (1)
380-439: Address high cyclomatic complexity.The pipeline failure indicates that the
handleSignalsmethod has a cyclomatic complexity of 17, which exceeds the limit of 15. Consider refactoring to reduce complexity.Extract individual signal handlers into separate methods:
func (cc *Conn) handleSignals(r *pool.Message) bool { switch r.Code() { case codes.CSM: - if cc.disablePeerTCPSignalMessageCSMs { - return true - } - if size, err := r.GetOptionUint32(message.TCPMaxMessageSize); err == nil { - cc.peerMaxMessageSize.Store(size) - } - if r.HasOption(message.TCPBlockWiseTransfer) { - cc.peerBlockWiseTranferEnabled.Store(true) - } - - // signal CSM message is received. - if cc.tcpSignalReceivedHandler != nil { - cc.tcpSignalReceivedHandler(codes.CSM) - } - return true + return cc.handleCSM(r) case codes.Ping: - // if r.HasOption(message.TCPCustody) { - // TODO - // } - if err := cc.sendPong(r.Token()); err != nil && !coapNet.IsConnectionBrokenError(err) { - cc.Session().errors(fmt.Errorf("cannot handle ping signal: %w", err)) - } - - if cc.tcpSignalReceivedHandler != nil { - cc.tcpSignalReceivedHandler(codes.Ping) - } - return true + return cc.handlePing(r) case codes.Release: - // if r.HasOption(message.TCPAlternativeAddress) { - // TODO - // } - - if cc.disablePeerTCPSignalMessageCSMs { - cc.tcpSignalReceivedHandler(codes.Release) - } - return true + return cc.handleRelease(r) case codes.Abort: - // if r.HasOption(message.TCPBadCSMOption) { - // TODO - // } - - if cc.disablePeerTCPSignalMessageCSMs { - cc.tcpSignalReceivedHandler(codes.Abort) - } - return true + return cc.handleAbort(r) case codes.Pong: - if h, ok := cc.tokenHandlerContainer.LoadAndDelete(r.Token().Hash()); ok { - cc.processReceivedMessage(r, cc, h) - } - - if cc.tcpSignalReceivedHandler != nil { - cc.tcpSignalReceivedHandler(codes.Pong) - } - return true + return cc.handlePong(r) } return false } +func (cc *Conn) handleCSM(r *pool.Message) bool { + if cc.disablePeerTCPSignalMessageCSMs { + return true + } + if size, err := r.GetOptionUint32(message.TCPMaxMessageSize); err == nil { + cc.peerMaxMessageSize.Store(size) + } + if r.HasOption(message.TCPBlockWiseTransfer) { + cc.peerBlockWiseTranferEnabled.Store(true) + } + if cc.tcpSignalReceivedHandler != nil { + cc.tcpSignalReceivedHandler(codes.CSM) + } + return true +} +// Add similar methods for handlePing, handleRelease, handleAbort, handlePong
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
options/tcpOptions.go(2 hunks)tcp/client.go(2 hunks)tcp/client/config.go(1 hunks)tcp/client/conn.go(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tcp/client.go (1)
message/codes/codes.go (2)
Code(12-12)CSM(53-53)
options/tcpOptions.go (1)
tcp/client/config.go (1)
Config(41-54)
🪛 GitHub Actions: Golangci-lint
tcp/client/conn.go
[error] 380-380: golangci-lint: cyclomatic complexity 17 of func (*Conn).handleSignals is high (> 15) (gocyclo)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Fuzzing
- GitHub Check: test (windows-latest)
🔇 Additional comments (9)
tcp/client/config.go (1)
53-53: LGTM!The addition of
CSMExchangeTimeoutfield to the Config struct is clean and appropriately typed for timeout configuration.options/tcpOptions.go (1)
5-5: LGTM!The addition of the
timeimport is necessary for the new timeout functionality.tcp/client.go (2)
10-10: LGTM!The import of
message/codesis necessary for the CSM code constant.
104-113: Potential race condition in CSM handler setup.There's a potential race condition where the CSM message could arrive and be processed before the
selectstatement in lines 124-129 is reached, causing the channel close to be missed.The current implementation creates the channel and sets up the handler before starting the connection goroutine, which is good. However, consider adding a safety mechanism to ensure the handler is properly synchronized with the waiting logic.
var csmExchangeDone chan struct{} if cfg.CSMExchangeTimeout != 0 && !cfg.DisablePeerTCPSignalMessageCSMs { csmExchangeDone = make(chan struct{}) + // Use sync.Once to ensure channel is closed only once + var closeOnce sync.Once cc.OnTCPSignalReceivedHandler(func(code codes.Code) { if code == codes.CSM { - close(csmExchangeDone) + closeOnce.Do(func() { close(csmExchangeDone) }) } }) }tcp/client/conn.go (5)
30-30: LGTM!The type alias for
TCPSignalReceivedHandleris well-defined and follows Go conventions.
56-56: LGTM!The addition of the
tcpSignalReceivedHandlerfield to theConnstruct is appropriate.
393-396: LGTM!The CSM signal handler invocation is correctly implemented with proper null checking.
406-408: LGTM!The Ping signal handler invocation is correctly implemented with proper null checking.
433-435: LGTM!The Pong signal handler invocation is correctly implemented with proper null checking.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tcp/client/conn.go (2)
275-279: LGTM with naming suggestion!The method correctly implements thread-safe handler registration using proper locking.
However, consider fixing the naming convention:
-func (cc *Conn) SetTCPSignalReceivedHandler(handler TCPSignalReceivedHandler) { +func (cc *Conn) SetTCPSignalReceivedHandler(handler TCPSignalReceivedHandler) {Actually the method name is fine - the static analysis warning is about the other method name.
384-390: Fix naming convention issue.The method logic is correct with proper synchronization and nil checking.
However, fix the naming convention as flagged by static analysis:
-func (cc *Conn) handleTcpSignalReceived(code codes.Code) { +func (cc *Conn) handleTCPSignalReceived(code codes.Code) {And update all calls to this method accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
options/tcpOptions.go(2 hunks)tcp/client.go(2 hunks)tcp/client/conn.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tcp/client.go
- options/tcpOptions.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
tcp/client/conn.go (2)
message/codes/codes.go (6)
Code(12-12)CSM(53-53)Ping(54-54)Pong(55-55)Release(56-56)Abort(57-57)mux/client.go (1)
Conn(17-51)
🪛 GitHub Check: lint
tcp/client/conn.go
[failure] 384-384:
var-naming: method handleTcpSignalReceived should be handleTCPSignalReceived (revive)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (windows-latest)
- GitHub Check: Fuzzing
🔇 Additional comments (8)
tcp/client/conn.go (8)
8-8: LGTM!The sync import is correctly added to support the new synchronization mechanisms.
31-31: LGTM!The handler type definition is clean and follows Go conventions for callback functions.
57-58: LGTM!The new fields are appropriately typed and positioned. The mutex will provide thread-safe access to the handler field.
404-406: LGTM!The CSM signal handler notification is correctly placed after processing the CSM message.
416-416: LGTM!The Ping signal handler notification is appropriately added.
418-424: LGTM!The Pong handling correctly preserves existing token handler logic while adding signal notification.
430-430: LGTM!The Release signal handler notification is correctly implemented.
437-437: LGTM!The Abort signal handler notification is correctly implemented.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
net/tlslistener_test.go (1)
107-109: Consider adding TLS MinVersion for better security.The TLS configuration lacks a minimum version specification, defaulting to potentially insecure older versions. While this may be acceptable for testing environments, consider adding explicit version control for better security practices.
Config: &tls.Config{ + MinVersion: tls.VersionTLS12, InsecureSkipVerify: true, Certificates: []tls.Certificate{cert}, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
net/conn_test.go(1 hunks)net/tlslistener_test.go(2 hunks)tcp/client.go(3 hunks)tcp/server_test.go(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.38.6)
net/tlslistener_test.go
[warning] 106-109: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
Certificates: []tls.Certificate{cert},
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🔇 Additional comments (7)
tcp/client.go (3)
35-39: LGTM: Modernized TLS dialing approach.The switch from
tls.DialWithDialerto usingtls.DialerwithDialContextprovides better context support and consistency with the rest of the codebase.
109-118: LGTM: Well-structured CSM exchange setup.The conditional CSM exchange setup is properly implemented:
- Only creates the channel and handler when CSM exchange is enabled
- Uses a buffered channel to prevent blocking
- Handler correctly identifies CSM messages and signals completion
127-141: LGTM: Proper timeout handling and cleanup.The CSM exchange timeout logic is well-implemented:
- Properly handles both timeout and successful completion cases
- Correctly closes connection and returns nil on timeout (addressing race condition)
- Cleans up the handler after exchange completion or timeout
- Error messaging is clear and informative
This addresses the race condition mentioned in the PR objectives by ensuring the client waits for CSM exchange before proceeding.
net/conn_test.go (1)
68-69: LGTM: Consistent dialing modernization.The switch to context-aware dialing using
net.DialerwithDialContextis consistent with the modernization pattern applied across the codebase.tcp/server_test.go (1)
304-305: LGTM: Consistent dialing modernization.The update to use
net.DialerwithDialContextmaintains consistency with the modernization pattern applied throughout the codebase.net/tlslistener_test.go (2)
103-112: LGTM: Modernized TLS dialing approach.The switch to
tls.DialerwithDialContextprovides better context support and is consistent with similar changes intcp/client.go.
193-194: LGTM: Consistent dialing modernization.The update to context-aware dialing using
net.Dialermaintains consistency with the modernization pattern applied across test files.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tcp/client_test.go (2)
844-844: Fix gofumpt whitespace warningsThere are unnecessary leading blank lines at Line 844 and Line 887, and the file fails gofumpt formatting. Please remove the extra blank lines and run gofumpt.
Apply minimal fixes:
- func TestConnWithCSMExchangeTimeout(t *testing.T) {And later around Line 887:
- l, err := coapNet.NewTCPListener("tcp", "")Also, run gofumpt to normalize formatting across the file.
Also applies to: 887-887
867-881: Speed up the timeout scenario to keep CI fastA 3s timeout slows the suite. Given local networking and the determinism of disabling CSM on the server, 1s is typically sufficient and reduces flakiness.
- options.WithCSMExchangeTimeout(time.Second * 3), + options.WithCSMExchangeTimeout(time.Second * 1),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tcp/client.go(3 hunks)tcp/client_test.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
tcp/client_test.go
[failure] 887-887:
unnecessary leading newline (whitespace)
[failure] 844-844:
unnecessary leading newline (whitespace)
[failure] 845-845:
File is not properly formatted (gofumpt)
🪛 GitHub Actions: Golangci-lint
tcp/client_test.go
[error] 845-845: File is not properly formatted (gofumpt). Step: golangci-lint run --timeout=5m
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (windows-latest)
- GitHub Check: Fuzzing
🔇 Additional comments (2)
tcp/client_test.go (1)
21-21: Import of tcp/server is appropriateNeeded for the server.Option type used in the new test. No issues.
tcp/client.go (1)
34-41: TLS dialing path update looks goodUsing tls.Dialer with DialContext respects context and NetDialer settings and is the preferred approach over tls.DialWithDialer. Nice improvement.
| client, err := Dial(l.Addr().String(), | ||
| tt.args.clientOptions...) | ||
| if tt.wantErr { | ||
| require.Nil(t, client) | ||
| require.Error(t, err) | ||
| } else { | ||
| require.NotNil(t, client) | ||
| require.NoError(t, err) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Prevent resource leaks and avoid package shadowing
- The variable name client shadows the imported client package; prefer cc for consistency with the rest of this file.
- On the success path the connection is never closed; this can leak goroutines and sockets. Close and wait for Done as in other tests.
- client, err := Dial(l.Addr().String(),
- tt.args.clientOptions...)
- if tt.wantErr {
- require.Nil(t, client)
- require.Error(t, err)
- } else {
- require.NotNil(t, client)
- require.NoError(t, err)
- }
+ cc, err := Dial(l.Addr().String(), tt.args.clientOptions...)
+ if tt.wantErr {
+ require.Nil(t, cc)
+ require.Error(t, err)
+ return
+ }
+ require.NoError(t, err)
+ require.NotNil(t, cc)
+ defer func() {
+ _ = cc.Close()
+ <-cc.Done()
+ }()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| client, err := Dial(l.Addr().String(), | |
| tt.args.clientOptions...) | |
| if tt.wantErr { | |
| require.Nil(t, client) | |
| require.Error(t, err) | |
| } else { | |
| require.NotNil(t, client) | |
| require.NoError(t, err) | |
| } | |
| }) | |
| cc, err := Dial(l.Addr().String(), tt.args.clientOptions...) | |
| if tt.wantErr { | |
| require.Nil(t, cc) | |
| require.Error(t, err) | |
| return | |
| } | |
| require.NoError(t, err) | |
| require.NotNil(t, cc) | |
| defer func() { | |
| _ = cc.Close() | |
| <-cc.Done() | |
| }() |
🤖 Prompt for AI Agents
In tcp/client_test.go around lines 907 to 916, the test shadows the imported
client package by naming the variable "client" and on the success path never
closes the connection which can leak goroutines/sockets; rename the local
variable to "cc" to match the rest of the file and, in the non-error branch,
close the connection and wait for its Done signal (same pattern used in other
tests) before returning from the subtest so resources are released.
| var csmExchangeDone chan struct{} | ||
| if cfg.CSMExchangeTimeout != 0 && !cfg.DisablePeerTCPSignalMessageCSMs { | ||
| csmExchangeDone = make(chan struct{}) | ||
|
|
||
| cc.SetTCPSignalReceivedHandler(func(code codes.Code) { | ||
| if code == codes.CSM { | ||
| close(csmExchangeDone) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| go func() { | ||
| err := cc.Run() | ||
| if err != nil { | ||
| cfg.Errors(fmt.Errorf("%v: %w", cc.RemoteAddr(), err)) | ||
| } | ||
| }() | ||
|
|
||
| return cc | ||
| // if CSM messages are enabled, wait for the CSM messages to be exchanged | ||
| if cfg.CSMExchangeTimeout != 0 && !cfg.DisablePeerTCPSignalMessageCSMs { | ||
| select { | ||
| case <-time.After(cfg.CSMExchangeTimeout): | ||
| err := fmt.Errorf("%v: timeout waiting for CSM exchange with peer", cc.RemoteAddr()) | ||
| cfg.Errors(err) | ||
| cc.Close() // Close connection on timeout | ||
| return nil, err // or return cc with an error state | ||
| case <-csmExchangeDone: | ||
| // CSM exchange completed successfully | ||
| } | ||
| // Clear the handler after exchange is complete or timed out | ||
| cc.SetTCPSignalReceivedHandler(nil) | ||
| } |
There was a problem hiding this comment.
Fix potential panic on close, ensure handler cleanup on all paths, and avoid timer leaks
Issues:
- Closing csmExchangeDone without guarding can panic if multiple CSM signals arrive (Line 114).
- Handler is not cleared on the timeout return path; defer the cleanup to cover all exits (Lines 137-139).
- time.After allocates a timer that can linger; prefer time.NewTimer and stop it (Lines 129-136).
- Consider exiting early if the connection closes before the exchange completes.
Proposed fix:
- var csmExchangeDone chan struct{}
- if cfg.CSMExchangeTimeout != 0 && !cfg.DisablePeerTCPSignalMessageCSMs {
- csmExchangeDone = make(chan struct{})
-
- cc.SetTCPSignalReceivedHandler(func(code codes.Code) {
- if code == codes.CSM {
- close(csmExchangeDone)
- }
- })
- }
+ var csmExchangeDone chan struct{}
+ var csmOnce sync.Once
+ if cfg.CSMExchangeTimeout != 0 && !cfg.DisablePeerTCPSignalMessageCSMs {
+ csmExchangeDone = make(chan struct{})
+ cc.SetTCPSignalReceivedHandler(func(code codes.Code) {
+ if code == codes.CSM {
+ csmOnce.Do(func() { close(csmExchangeDone) })
+ }
+ })
+ // Ensure handler is always cleared, regardless of outcome.
+ defer cc.SetTCPSignalReceivedHandler(nil)
+ }
@@
- // if CSM messages are enabled, wait for the CSM messages to be exchanged
- if cfg.CSMExchangeTimeout != 0 && !cfg.DisablePeerTCPSignalMessageCSMs {
- select {
- case <-time.After(cfg.CSMExchangeTimeout):
- err := fmt.Errorf("%v: timeout waiting for CSM exchange with peer", cc.RemoteAddr())
- cfg.Errors(err)
- cc.Close() // Close connection on timeout
- return nil, err // or return cc with an error state
- case <-csmExchangeDone:
- // CSM exchange completed successfully
- }
- // Clear the handler after exchange is complete or timed out
- cc.SetTCPSignalReceivedHandler(nil)
- }
+ // If enabled, wait for the CSM exchange to complete or time out.
+ if cfg.CSMExchangeTimeout != 0 && !cfg.DisablePeerTCPSignalMessageCSMs {
+ timer := time.NewTimer(cfg.CSMExchangeTimeout)
+ defer timer.Stop()
+ select {
+ case <-timer.C:
+ err := fmt.Errorf("%v: timeout waiting for CSM exchange with peer", cc.RemoteAddr())
+ cfg.Errors(err)
+ _ = cc.Close()
+ return nil, err
+ case <-cc.Done():
+ return nil, fmt.Errorf("%v: connection closed before CSM exchange: %w", cc.RemoteAddr(), cc.Context().Err())
+ case <-csmExchangeDone:
+ // CSM exchange completed successfully
+ }
+ }And add the import:
import (
"crypto/tls"
"fmt"
"net"
"time"
+ "sync"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var csmExchangeDone chan struct{} | |
| if cfg.CSMExchangeTimeout != 0 && !cfg.DisablePeerTCPSignalMessageCSMs { | |
| csmExchangeDone = make(chan struct{}) | |
| cc.SetTCPSignalReceivedHandler(func(code codes.Code) { | |
| if code == codes.CSM { | |
| close(csmExchangeDone) | |
| } | |
| }) | |
| } | |
| go func() { | |
| err := cc.Run() | |
| if err != nil { | |
| cfg.Errors(fmt.Errorf("%v: %w", cc.RemoteAddr(), err)) | |
| } | |
| }() | |
| return cc | |
| // if CSM messages are enabled, wait for the CSM messages to be exchanged | |
| if cfg.CSMExchangeTimeout != 0 && !cfg.DisablePeerTCPSignalMessageCSMs { | |
| select { | |
| case <-time.After(cfg.CSMExchangeTimeout): | |
| err := fmt.Errorf("%v: timeout waiting for CSM exchange with peer", cc.RemoteAddr()) | |
| cfg.Errors(err) | |
| cc.Close() // Close connection on timeout | |
| return nil, err // or return cc with an error state | |
| case <-csmExchangeDone: | |
| // CSM exchange completed successfully | |
| } | |
| // Clear the handler after exchange is complete or timed out | |
| cc.SetTCPSignalReceivedHandler(nil) | |
| } | |
| var csmExchangeDone chan struct{} | |
| var csmOnce sync.Once | |
| if cfg.CSMExchangeTimeout != 0 && !cfg.DisablePeerTCPSignalMessageCSMs { | |
| csmExchangeDone = make(chan struct{}) | |
| cc.SetTCPSignalReceivedHandler(func(code codes.Code) { | |
| if code == codes.CSM { | |
| csmOnce.Do(func() { close(csmExchangeDone) }) | |
| } | |
| }) | |
| // Ensure handler is always cleared, regardless of outcome. | |
| defer cc.SetTCPSignalReceivedHandler(nil) | |
| } | |
| go func() { | |
| err := cc.Run() | |
| if err != nil { | |
| cfg.Errors(fmt.Errorf("%v: %w", cc.RemoteAddr(), err)) | |
| } | |
| }() | |
| // If enabled, wait for the CSM exchange to complete or time out. | |
| if cfg.CSMExchangeTimeout != 0 && !cfg.DisablePeerTCPSignalMessageCSMs { | |
| timer := time.NewTimer(cfg.CSMExchangeTimeout) | |
| defer timer.Stop() | |
| select { | |
| case <-timer.C: | |
| err := fmt.Errorf("%v: timeout waiting for CSM exchange with peer", cc.RemoteAddr()) | |
| cfg.Errors(err) | |
| _ = cc.Close() | |
| return nil, err | |
| case <-cc.Done(): | |
| return nil, fmt.Errorf("%v: connection closed before CSM exchange: %w", cc.RemoteAddr(), cc.Context().Err()) | |
| case <-csmExchangeDone: | |
| // CSM exchange completed successfully | |
| } | |
| } |
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tcp/client_test.go (1)
905-914: Fix resource leak and avoid shadowing the imported client package; also assert timeout error type
- The local variable name client shadows the imported client package, which harms readability.
- On the success path the connection is never closed, leaking a socket and goroutines.
- For the timeout scenario, assert the expected error kind to lock in behavior.
Apply this diff:
- client, err := Dial(l.Addr().String(), - tt.args.clientOptions...) - if tt.wantErr { - require.Nil(t, client) - require.Error(t, err) - } else { - require.NotNil(t, client) - require.NoError(t, err) - } + cc, err := Dial(l.Addr().String(), tt.args.clientOptions...) + if tt.wantErr { + require.Nil(t, cc) + require.Error(t, err) + // CSM-exchange timeout should surface as a deadline exceeded. + require.ErrorIs(t, err, context.DeadlineExceeded) + return + } + require.NoError(t, err) + require.NotNil(t, cc) + defer func() { + _ = cc.Close() + <-cc.Done() + }()
🧹 Nitpick comments (1)
tcp/client_test.go (1)
844-883: Keep tests snappy and precise: shorten timeouts and parallelize subtests
- A 3s CSM timeout will slow CI; consider tightening to a few hundred milliseconds for local loopback tests.
- Optionally run subtests in parallel; each subtest uses its own listener/server, so it’s safe and speeds up the suite.
Apply this diff to reduce test duration and parallelize:
@@ func TestConnWithCSMExchangeTimeout(t *testing.T) { @@ - { + { name: "client-server-csm-success", args: args{ clientOptions: []Option{ - options.WithCSMExchangeTimeout(time.Second * 3), + options.WithCSMExchangeTimeout(300 * time.Millisecond), }, }, wantErr: false, }, { name: "client-server-csm-timeout", args: args{ clientOptions: []Option{ - options.WithCSMExchangeTimeout(time.Second * 3), + options.WithCSMExchangeTimeout(300 * time.Millisecond), }, serverOptions: []server.Option{ options.WithDisableTCPSignalMessageCSM(), }, }, wantErr: true, }, @@ for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tcp/client_test.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tcp/client_test.go (4)
tcp/client.go (2)
Option(20-22)Dial(25-47)options/tcpOptions.go (2)
WithCSMExchangeTimeout(50-54)WithDisableTCPSignalMessageCSM(38-40)net/tcplistener.go (1)
NewTCPListener(31-37)tcp/server.go (1)
NewServer(7-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (windows-latest)
- GitHub Check: Fuzzing
🔇 Additional comments (1)
tcp/client_test.go (1)
21-21: LGTM: server import is necessary for typed server optionsImporting tcp/server enables the use of server.Option in the new CSM test. Looks good.
Summary by CodeRabbit
New Features
Improvements
Breaking Changes
Tests