prevent port number reuse with TTL-based caching #4689
Conversation
WalkthroughThe changes introduce a new Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
pkg/lib/network/ports_test.go (2)
38-51: Potential for test flakiness due to short TTLIn
TestPortReservation, using a TTL of 1 second may lead to flakiness on slower systems where the port might be released before the second allocation. Consider increasing the TTL or adding a brief delay between allocations to ensure consistent test results.Apply this diff to include a delay:
func (s *PortAllocatorTestSuite) TestPortReservation() { // Create allocator with 1 second TTL for testing allocator := network.NewPortAllocator(time.Second) // Get first port port1, err := allocator.GetFreePort() s.Require().NoError(err) + // Wait briefly to ensure the port isn't reused + time.Sleep(100 * time.Millisecond) // Get second port - should be different port2, err := allocator.GetFreePort() s.Require().NoError(err) s.NotEqual(port1, port2, "got same port within TTL period") }
Line range hint
98-109: Robustness concern with potential port exhaustionIn
TestGlobalAllocator, allocating multiple ports in a tight loop could risk port exhaustion on systems with limited available ports or stringent firewall settings. Consider adding error handling for port allocation failures or reducing the number of iterations to improve test reliability.Apply this diff to handle potential errors gracefully:
func (s *PortAllocatorTestSuite) TestGlobalAllocator() { // Get a batch of ports using the global allocator usedPorts := make(map[int]bool) for i := 0; i < 10; i++ { port, err := network.GetFreePort() if err != nil { + // Skip iteration if port allocation fails + continue } s.Require().NoError(err) s.False(usedPorts[port], "global allocator reused port %d", port) usedPorts[port] = true } }pkg/lib/network/ports.go (2)
10-11: Consider making the default TTL configurable or documenting the choice.The
defaultPortAllocatorTTLis set to5 * time.Second. While this value may work in most cases, different environments might benefit from a configurable TTL. Alternatively, ensure that this default value is well-documented to inform users of its implications on port allocation timing.
91-103: EnsureIsPortOpenaccurately reflects port availability.The
IsPortOpenfunction attempts to listen on the specified port to check its availability. While effective, this method can have side effects in concurrent environments. Consider documenting any limitations or exploring non-intrusive ways to check port status.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
pkg/lib/network/ports.go(2 hunks)pkg/lib/network/ports_test.go(1 hunks)
🔇 Additional comments (6)
pkg/lib/network/ports_test.go (5)
17-19: Appropriate renaming of the test suite
Renaming the test suite to PortAllocatorTestSuite accurately reflects the expanded scope of the tests and enhances code clarity.
25-26: Descriptive comment enhances test understanding
Adding the comment to TestGetFreePort improves readability by clearly stating the purpose of the test.
31-35: Verification step strengthens the test
By verifying that the allocated port can be listened on, the test ensures that the port is truly available, adding robustness to the test.
53-80: Effective concurrent port allocation test
The TestConcurrentPortAllocation function properly tests thread safety by using a mutex to protect shared access to the ports map and by checking that no port is allocated multiple times.
82-96: Comprehensive validation of port availability
TestIsPortOpen effectively checks the IsPortOpen function by verifying port availability before and after binding, ensuring accurate detection of port states.
pkg/lib/network/ports.go (1)
39-40: Handle potential race conditions with the global allocator.
When using globalAllocator, concurrent calls to GetFreePort() might lead to contention. Verify that PortAllocator properly handles concurrent access without leading to deadlocks or race conditions.
Run the following script to check for race conditions:
✅ Verification successful
Race conditions are properly handled in the PortAllocator implementation
The PortAllocator implementation has proper synchronization mechanisms in place:
- It uses a
sync.Mutexto protect concurrent access to thereservedPortsmap:
type PortAllocator struct {
mu sync.Mutex
reservedPorts map[int]time.Time
ttl time.Duration
}- The
GetFreePort()method correctly acquires and releases the lock:
func (pa *PortAllocator) GetFreePort() (int, error) {
pa.mu.Lock()
defer pa.mu.Unlock()
// ... rest of implementation
}- The implementation has been tested for concurrent access in
TestConcurrentPortAllocationwhich verifies thread-safety by spawning multiple goroutines that allocate ports simultaneously.
The synchronization implementation ensures that:
- Only one goroutine can modify the
reservedPortsmap at a time - Port allocation and reservation are atomic operations
- The TTL-based port reservation system is thread-safe
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Run tests with the race detector enabled to check for race conditions.
# Test: Execute go test with the race flag. Expect: No race conditions reported.
go test -race ./pkg/lib/network
Length of output: 103
Script:
#!/bin/bash
# Let's examine the PortAllocator implementation to understand its thread safety
ast-grep --pattern 'type PortAllocator struct {
$$$
}'
# Also check the GetFreePort implementation
ast-grep --pattern 'func ($_) GetFreePort() $_'
# Look for any sync.Mutex or other synchronization primitives
rg "sync\." -A 3 -B 3
Length of output: 27771
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This reverts commit 858d44b.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
pkg/lib/network/ports.go (2)
19-25: Consider implementing periodic cleanup of expired reservations.While the cleanup of expired reservations during allocation is good, consider implementing a periodic cleanup mechanism (e.g., using a ticker) to prevent unbounded growth of the
reservedPortsmap in long-running systems, especially during periods of low allocation activity.Would you like me to provide an implementation of a background cleanup mechanism?
48-51: Document thread safety guarantees of the global allocator.While the implementation is thread-safe, it would be helpful to explicitly document the thread safety guarantees of the global allocator for users of this package.
Add a comment about thread safety:
var ( - // globalAllocator is the package-level port allocator instance used by GetFreePort + // globalAllocator is the package-level port allocator instance used by GetFreePort. + // It is safe for concurrent use by multiple goroutines. globalAllocator = NewPortAllocator(defaultPortAllocatorTTL) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
pkg/lib/network/ports.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: build / Build Binary (windows, amd64)
pkg/lib/network/ports.go
[failure] 6-6:
"sync" imported and not used
[failure] 15-15:
undefined: PortAllocator
[failure] 55-55:
undefined: PortAllocator
[failure] 56-56:
undefined: PortAllocator
[failure] 72-72:
undefined: PortAllocator
🪛 GitHub Check: build / Build Binary (darwin, arm64)
pkg/lib/network/ports.go
[failure] 6-6:
"sync" imported and not used
[failure] 15-15:
undefined: PortAllocator
[failure] 55-55:
undefined: PortAllocator
[failure] 56-56:
undefined: PortAllocator
[failure] 72-72:
undefined: PortAllocator
🪛 GitHub Check: build / Build Binary (darwin, amd64)
pkg/lib/network/ports.go
[failure] 6-6:
"sync" imported and not used
[failure] 15-15:
undefined: PortAllocator
[failure] 55-55:
undefined: PortAllocator
[failure] 56-56:
undefined: PortAllocator
[failure] 72-72:
undefined: PortAllocator
🪛 GitHub Check: build / Build Binary (linux, armv6)
pkg/lib/network/ports.go
[failure] 6-6:
"sync" imported and not used
[failure] 15-15:
undefined: PortAllocator
[failure] 55-55:
undefined: PortAllocator
[failure] 56-56:
undefined: PortAllocator
[failure] 72-72:
undefined: PortAllocator
🪛 GitHub Check: build / Build Binary (linux, armv7)
pkg/lib/network/ports.go
[failure] 6-6:
"sync" imported and not used
[failure] 15-15:
undefined: PortAllocator
[failure] 55-55:
undefined: PortAllocator
[failure] 56-56:
undefined: PortAllocator
[failure] 72-72:
undefined: PortAllocator
🪛 GitHub Check: build / Build Binary (linux, arm64)
pkg/lib/network/ports.go
[failure] 6-6:
"sync" imported and not used
[failure] 15-15:
undefined: PortAllocator
[failure] 55-55:
undefined: PortAllocator
[failure] 56-56:
undefined: PortAllocator
[failure] 72-72:
undefined: PortAllocator
🪛 GitHub Check: lint / go-lint (ubuntu-latest)
pkg/lib/network/ports.go
[failure] 6-6:
"sync" imported and not used
[failure] 15-15:
undefined: PortAllocator
[failure] 55-55:
undefined: PortAllocator
[failure] 56-56:
undefined: PortAllocator
[failure] 72-72:
undefined: PortAllocator (typecheck)
[failure] 6-6:
"sync" imported and not used
[failure] 15-15:
undefined: PortAllocator
[failure] 55-55:
undefined: PortAllocator
[failure] 56-56:
undefined: PortAllocator
🪛 GitHub Check: build / Build Binary (linux, amd64)
pkg/lib/network/ports.go
[failure] 6-6:
"sync" imported and not used
[failure] 15-15:
undefined: PortAllocator
[failure] 55-55:
undefined: PortAllocator
[failure] 56-56:
undefined: PortAllocator
[failure] 72-72:
undefined: PortAllocator
🔇 Additional comments (1)
pkg/lib/network/ports.go (1)
118-129: LGTM! Clear implementation with good documentation.
The IsPortOpen implementation correctly checks port availability and includes appropriate documentation about potential race conditions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
pkg/lib/network/ports.go (1)
104-115: Add timeout to prevent hanging on blocked ports.While the implementation is correct, it could benefit from a timeout to prevent hanging when checking ports that might be in TIME_WAIT or similar states.
func IsPortOpen(port int) bool { addr := net.JoinHostPort("", strconv.Itoa(port)) - ln, err := net.Listen("tcp", addr) + ln, err := net.ListenTimeout("tcp", addr, 100*time.Millisecond) if err != nil { return false } ln.Close() return true }pkg/lib/network/ports_test.go (6)
21-31: Consider using cleanup for the listener.While the test is well-structured, consider using
s.T().Cleanup()instead ofdeferto ensure the listener is closed even if the test fails.func (s *PortAllocatorTestSuite) TestGetFreePort() { port, err := GetFreePort() s.Require().NoError(err) s.NotEqual(0, port, "expected a non-zero port") // Verify we can listen on the port l, err := net.Listen("tcp", ":"+strconv.Itoa(port)) s.Require().NoError(err) - defer l.Close() + s.T().Cleanup(func() { + l.Close() + }) }
33-58: Consider using constants for test ports.The test effectively validates the TTL-based eviction mechanism, which is crucial for preventing port reuse. However, consider defining the test ports as constants to improve maintainability.
+const ( + testPort1 = 8080 + testPort2 = 8081 + testPort3 = 8082 +) func (s *PortAllocatorTestSuite) TestEvictionAndReservation() { now := time.Now() allocator := &PortAllocator{ reservedPorts: map[int]time.Time{ - 8080: now.Add(-time.Second), // expired - 8081: now.Add(time.Second), // not expired - 8082: now.Add(-time.Second), // expired + testPort1: now.Add(-time.Second), // expired + testPort2: now.Add(time.Second), // not expired + testPort3: now.Add(-time.Second), // expired },
60-78: Consider optimizing the test for faster execution.While the test effectively validates the retry limit, reserving all possible ports (64512 ports) might slow down the test suite. Consider testing with a smaller range of ports by modifying the PortAllocator to accept a port range for testing.
func (s *PortAllocatorTestSuite) TestMaxAttempts() { allocator := &PortAllocator{ reservedPorts: make(map[int]time.Time), ttl: time.Second, maxAttempts: 3, + // Add these fields for testing + minPort: 1024, + maxPort: 1034, // Test with just 10 ports } - // Reserve all possible user ports (1024-65535) - for i := 1024; i <= 65535; i++ { + // Reserve all ports in the test range + for i := allocator.minPort; i <= allocator.maxPort; i++ { allocator.reservedPorts[i] = time.Now().Add(time.Minute) }
80-107: LGTM! Excellent concurrency testing.The test effectively validates thread-safety using proper synchronization primitives. Consider extracting the number of concurrent goroutines (20) as a named constant for better maintainability.
109-122: Maintain consistency in cleanup approach.For consistency with the earlier suggestion, consider using
s.T().Cleanup()here as well.l, err := net.Listen("tcp", ":"+strconv.Itoa(port)) s.Require().NoError(err) - defer l.Close() + s.T().Cleanup(func() { + l.Close() + })
124-133: Consider enhancing global allocator test coverage.While the test verifies basic port reuse prevention, consider:
- Extracting the iteration count (3) as a named constant
- Adding a parallel test case using
t.Parallel()to verify the global allocator's behavior under concurrent test executionExample parallel test:
func (s *PortAllocatorTestSuite) TestGlobalAllocatorParallel() { s.T().Parallel() port1, err := GetFreePort() s.Require().NoError(err) port2, err := GetFreePort() s.Require().NoError(err) s.NotEqual(port1, port2, "ports should be different even in parallel tests") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
pkg/lib/network/ports.go(2 hunks)pkg/lib/network/ports_test.go(1 hunks)
🔇 Additional comments (5)
pkg/lib/network/ports.go (4)
11-14: LGTM! Constants are well-chosen.
The default TTL of 5 seconds provides sufficient time for service startup while the maximum of 10 attempts balances between retrying and failing fast.
16-29: LGTM! Well-designed thread-safe implementation.
The PortAllocator type is well-documented and properly implements thread-safe port allocation with TTL-based caching. The global instance with default values ensures backward compatibility.
45-50: LGTM! Clean wrapper implementation.
The package-level GetFreePort function provides a clean interface to the global allocator while maintaining backward compatibility.
52-80: LGTM! Robust implementation with proper cleanup.
The implementation includes all necessary elements for reliable port allocation:
- Proper mutex handling
- Cleanup of expired reservations
- Clear retry limits with descriptive errors
pkg/lib/network/ports_test.go (1)
1-20: LGTM! Well-structured test suite setup.
The test suite follows testify conventions and includes all necessary imports.
Problem
In devstack and tests, we start multiple services (API, NATS, publishers) that each need unique ports. Since we allocate all ports before starting the services, calling GetFreePort multiple times in quick succession could return the same port number, leading to service startup failures.
Solution
Added TTL-based port caching (5s default) to prevent GetFreePort from returning recently allocated ports. This gives devstack time to actually bind the ports before they can be reused.
The change is backwards compatible - existing GetFreePort calls automatically get the new caching behavior.
Summary by CodeRabbit
New Features
Bug Fixes
Tests