Conversation
connection establishment Reorder operations in OpenConn to register the connection before waiting for peer availability. This ensures: - Connection is ready to receive messages before peer subscription completes - Transport messages and onconnected events maintain proper ordering - No messages are lost during the connection establishment window - Concurrent OpenConn calls cannot create duplicate connections If peer availability check fails, the pre-registered connection is properly cleaned up.
Ensure relay connections are properly cleaned up when the service is not running by verifying `serviceIsRunning` and removing stale entries from `c.conns` to prevent unintended behaviors.
…from Client Conn previously held a direct *Client pointer and called client methods (writeTo, closeConn, LocalAddr) directly, creating a tight bidirectional coupling. The message channel was also created externally in OpenConn and shared between Conn and connContainer with unclear ownership. Now connContainer fully owns the lifecycle of both the channel and the Conn it wraps: - connContainer creates the channel (sized by connChannelSize const) and the Conn internally via newConnContainer - connContainer feeds messages into the channel (writeMsg), closes and drains it on shutdown (close) - Conn reads from the channel (Read) but never closes it Conn is decoupled from *Client by replacing the *Client field with three function closures (writeFn, closeFn, localAddrFn) that are wired by newConnContainer at construction time. Write, Close, and LocalAddr delegate to these closures. This removes the direct dependency while keeping the identity-check logic: writeTo and closeConn now compare connContainer pointers instead of Conn pointers to verify the caller is the current active connection for that peer.
📝 WalkthroughWalkthroughThe PR refactors relay client connection handling by introducing a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shared/relay/client/conn.go (1)
57-60:⚠️ Potential issue | 🟡 MinorCopy-paste bug in panic message: says "SetReadDeadline" instead of "SetWriteDeadline".
Line 59 panic string is incorrect — it should reference
SetWriteDeadline.Proposed fix
func (c *Conn) SetWriteDeadline(t time.Time) error { //TODO implement me - panic("SetReadDeadline is not implemented") + panic("SetWriteDeadline is not implemented") }
🧹 Nitpick comments (2)
shared/relay/client/conn.go (1)
24-33: Read: silent data truncation whenbis smaller thanm.Payload.If the caller provides a buffer smaller than the incoming payload,
copywill silently truncate data with no indication to the caller. This is a pre-existing concern (not introduced by this PR), but worth noting: callers must ensure buffer sizes match the expected MTU. Consider returningio.ErrShortBufferwhenlen(b) < len(m.Payload)in a follow-up.shared/relay/client/client.go (1)
21-21: Consider documenting the rationale forconnChannelSize = 100.A brief comment explaining why 100 was chosen (e.g., based on expected throughput, backpressure tolerance) would help future maintainers understand the trade-off between memory usage and message drop likelihood (given the
defaultdrop inwriteMsg).
2 participants
Refactor/relay conn container from Client
Conn previously held a direct *Client pointer and called client methods (writeTo, closeConn, LocalAddr) directly, creating a tight bidirectional coupling. The message channel was also created externally in OpenConn and shared between Conn and connContainer with unclear ownership.
Now connContainer fully owns the lifecycle of both the channel and the Conn it wraps:
Conn is decoupled from *Client by replacing the *Client field with three function closures (writeFn, closeFn, localAddrFn) that are wired by newConnContainer at construction time. Write, Close, and LocalAddr delegate to these closures. This removes the direct dependency while keeping the identity-check logic: writeTo and closeConn now compare connContainer pointers instead of Conn pointers to verify the caller is the current active connection for that peer.
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit