-
-
Notifications
You must be signed in to change notification settings - Fork 798
[client] Fix race issues in lazy tests #4181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes race conditions in lazy connection tests by adding proper synchronization when accessing peer listeners. The changes introduce a thread-safe method to retrieve peer listeners and update test code to use this method instead of directly accessing the internal peers map.
- Adds a
GetPeerListener
method with proper mutex locking for thread-safe access - Updates test functions to use the new safe accessor method instead of direct map access
- Improves error handling by checking for peer listener existence before use
if err := trigger(mgr.peers[peerCfg2.PeerConnID].conn.LocalAddr().String()); err != nil { | ||
listener, exists = mgr.GetPeerListener(peerCfg2.PeerConnID) | ||
if !exists { | ||
t.Fatalf("peer listener for peer1 not found") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message incorrectly refers to 'peer1' but should refer to 'peer2' since this is checking for peerCfg2.PeerConnID.
t.Fatalf("peer listener for peer1 not found") | |
t.Fatalf("peer listener for peer2 not found") |
Copilot uses AI. Check for mistakes.
@@ -33,6 +33,15 @@ func (m MocWGIface) UpdatePeer(string, []netip.Prefix, time.Duration, *net.UDPAd | |||
|
|||
} | |||
|
|||
// Add this method to the Manager struct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment suggests adding the method to the Manager struct, but the method is actually being added to the test file. The comment should clarify that this is a test helper method or be moved to the actual Manager struct implementation.
Copilot uses AI. Check for mistakes.
|
Describe your changes
Fix race issues in lazy tests
Issue ticket number and link
Stack
Checklist