feat: support reusePort on server listen #115
Conversation
|
Warning Rate limit exceeded@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdd a public reusePort option (default false), wire platform-aware reusePort handling into master, app-worker, and worker_threads forking/listening, add tests and a clustered reuseport script, update CI/packaging metadata and README, and refine agent/app-worker logging and startup error handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Master
participant Options
participant Forker as Forker/ModeImpl
participant AppWorker
participant Kernel as OS/Kernel
Note over Master,Options: Start cluster with reusePort config
Master->>Options: read cluster config (includes reusePort)
Options->>Options: normalize + platform support check
alt platform unsupported
Options-->>Master: reusePort = false (override)
else platform supports reusePort
Options-->>Master: reusePort = true/false (as configured)
end
Master->>Forker: start forking (process or worker_threads)
alt reusePort = true
Forker->>AppWorker: fork N workers (same port, distinct appWorkerId/argv)
else reusePort = false
Forker->>AppWorker: fork per-port workers (existing behavior)
end
AppWorker->>Kernel: server.listen({ port, reusePort: <bool>, host? })
Kernel-->>AppWorker: socket bound (SO_REUSEPORT applied if true)
AppWorker-->>Master: send listening/start message (via cluster IPC when reusePort)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (7)
test/fixtures/apps/app-listen-reusePort/app/router.js (1)
6-8: Consider adding a comment about private API usage.While accessing
_optionsis acceptable in test fixtures, it would be helpful to add a comment explaining why we're accessing this private API to prevent confusion for other developers.app.get('/port', ctx => { + // Note: Accessing private _options API for testing reusePort functionality ctx.body = ctx.app._options.port; });lib/utils/options.js (1)
15-15: LGTM with suggestions for improvement.The
reusePortoption is correctly implemented with a safe default value offalse. However, consider these improvements:
- Add JSDoc comments explaining the purpose and security implications of this option
- Add validation similar to other critical options
Apply this diff to add documentation and validation:
port: options.https ? 8443 : null, + // Enable SO_REUSEPORT socket option for better performance with multiple workers. + // Warning: This allows multiple processes to bind to the same port. + // See: https://lwn.net/Articles/542629/ reusePort: false, workers: null,And add this validation after the port parsing:
options.port = parseInt(options.port, 10) || undefined; options.workers = parseInt(options.workers, 10); + assert(typeof options.reusePort === 'boolean', 'options.reusePort must be a boolean'); if (options.require) options.require = [].concat(options.require);lib/app_worker.js (1)
130-130: Maintain consistent debug message format.The debug messages use different string formats for the options:
debug('listen options %s', listenOptions)uses an objectdebug('listen options %s', args)uses an arrayConsider using the same format for consistency:
-debug('listen options %s', args); +debug('listen options %j', { port, hostname: listenConfig.hostname });Also applies to: 137-137
test/app_worker.test.js (1)
235-262: LGTM with suggestions for enhanced reusePort testing.The test case follows the established pattern and verifies basic functionality. However, consider enhancing the test to specifically validate the reusePort behavior.
Consider adding these specific test scenarios:
// Verify reusePort is actually set await request('http://127.0.0.1:17010') .get('/reusePort') .expect('true') .expect(200); // Test multiple workers can bind to same port const responses = await Promise.all([ request('http://127.0.0.1:17010').get('/pid'), request('http://127.0.0.1:17010').get('/pid'), ]); assert(responses[0].text !== responses[1].text, 'Requests should be handled by different workers');This would require adding corresponding endpoints in your test app to expose:
- The reusePort setting status
- The worker process ID handling each request
lib/master.js (2)
37-37: Enhance documentation with performance and security notes.While the documentation is comprehensive regarding platform compatibility, consider adding:
- Performance implications of using reusePort
- Security considerations, especially in multi-tenant environments
- * - {Boolean} [reusePort] setting `reusePort` to `true` allows multiple sockets on the same host to bind to the same port. Incoming connections are distributed by the operating system to listening sockets. This option is available only on some platforms, such as Linux 3.9+, DragonFlyBSD 3.6+, FreeBSD 12.0+, Solaris 11.4, and AIX 7.2.5+. **Default:** `false`. + * - {Boolean} [reusePort] setting `reusePort` to `true` allows multiple sockets on the same host to bind to the same port. Incoming connections are distributed by the operating system to listening sockets, which can improve performance under high connection loads. This option is available only on some platforms, such as Linux 3.9+, DragonFlyBSD 3.6+, FreeBSD 12.0+, Solaris 11.4, and AIX 7.2.5+. Note: In multi-tenant environments, ensure proper isolation as all workers can bind to the same port. **Default:** `false`.
Line range hint
447-467: Add platform compatibility check for reusePort.The code should verify platform support for SO_REUSEPORT at runtime to prevent startup failures on unsupported platforms.
Here's a suggested implementation:
startMasterSocketServer(cb) { + // Check platform support for reusePort + if (this.options.reusePort) { + const isSupported = process.versions.node.split('.')[0] >= 12 && + ['linux', 'freebsd', 'darwin'].includes(process.platform); + if (!isSupported) { + this.logger.warn( + '[master] reusePort is not supported on this platform. Falling back to default behavior.' + ); + this.options.reusePort = false; + } + } + // Create the outside facing server listening on our port. require('net').createServer({ pauseOnConnect: true, + reusePort: this.options.reusePort, }, connection => {test/master.test.js (1)
32-41: LGTM with suggestions for additional test coverage.The test case correctly verifies the basic startup scenario with
reusePort=true. However, consider adding more test cases to ensure comprehensive coverage:
- Verify that multiple workers can actually bind to the same port
- Test error scenarios (e.g., when port is already in use)
- Test port reuse across worker restarts
Here's a suggested additional test case:
it('should allow multiple workers to bind to same port with reusePort=true', async () => { app = utils.cluster('apps/master-worker-started', { workers: 4, reusePort: true }); await app.expect('stdout', /egg start/) .expect('stdout', /egg started/) .expect('code', 0) .end(); // Verify each worker bound successfully for (let i = 1; i <= 4; i++) { app.expect('stdout', new RegExp(`worker#${i}.*port bound successfully`)); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- .github/PULL_REQUEST_TEMPLATE.md (0 hunks)
- .github/workflows/nodejs.yml (1 hunks)
- README.md (1 hunks)
- lib/app_worker.js (2 hunks)
- lib/master.js (1 hunks)
- lib/utils/options.js (1 hunks)
- test/app_worker.test.js (1 hunks)
- test/fixtures/apps/app-listen-reusePort/app.js (1 hunks)
- test/fixtures/apps/app-listen-reusePort/app/router.js (1 hunks)
- test/fixtures/apps/app-listen-reusePort/config/config.default.js (1 hunks)
- test/fixtures/apps/app-listen-reusePort/package.json (1 hunks)
- test/master.test.js (1 hunks)
💤 Files with no reviewable changes (1)
- .github/PULL_REQUEST_TEMPLATE.md
✅ Files skipped from review due to trivial changes (2)
- README.md
- test/fixtures/apps/app-listen-reusePort/package.json
🧰 Additional context used
🪛 Biome
lib/app_worker.js
[error] 38-38: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
test/fixtures/apps/app-listen-reusePort/app.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
test/fixtures/apps/app-listen-reusePort/config/config.default.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (8)
test/fixtures/apps/app-listen-reusePort/app.js (2)
1-2: Keep the 'use strict' directive.While modules are automatically in strict mode and the static analysis flags this as redundant, it's common practice in Node.js projects to keep explicit strict mode declarations for consistency and backwards compatibility.
🧰 Tools
🪛 Biome
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
3-6: 🛠️ Refactor suggestionConsider using a public API to configure the port.
The code directly modifies the internal
_optionsproperty, which could be fragile if the internal API changes. Consider checking if there's a public API method to configure the port.Let's check if there's a public API available:
If a public API exists, consider refactoring to:
- app._options.port = undefined; + app.setPort(undefined); // or equivalent public API✅ Verification successful
Based on the search results and the context from the test files, I can now generate the final response:
The direct modification of
_options.portis the intended way in test fixturesThe code is in a test fixture that deliberately unsets the port to test specific server binding scenarios. This is a valid use case because:
- The port configuration is primarily handled through the cluster configuration in
config.default.jsfiles, as seen in multiple test fixtures.- The
_optionsproperty is part of the framework's internal API used consistently across test files for port configuration.- Test fixtures like
app-listen-reusePort,app-listen-without-port, and others follow the same pattern to test different port binding scenarios.No public API exists specifically for modifying the port after initialization, as this is typically configured through the application's configuration files rather than programmatically.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for port configuration methods in the codebase rg -A 5 "prototype.*[pP]ort"Length of output: 30
Script:
#!/bin/bash # Let's try a broader search for port-related configurations and methods rg -i "\.port" -A 5 # Also search for port-related configurations in the test files rg -i "port.*=|set.*port" test/ -A 5 # Look for Application class definition and port-related methods ast-grep --pattern 'class Application { $$$ }'Length of output: 46714
Script:
#!/bin/bash # Let's search for port configuration in the Application class and related files rg -l "class.*Application" | xargs rg -A 5 "port" # Look for port configuration methods in lib directory rg -A 5 "prototype.*port|set.*port" lib/ # Search for configuration documentation rg -i "port.*configuration|configure.*port" -g "*.md"Length of output: 975
test/fixtures/apps/app-listen-reusePort/config/config.default.js (1)
3-11: LGTM! Configuration matches PR requirements.The configuration correctly implements the reusePort feature as specified in issue #5365. The port number is appropriate for testing purposes.
Note: While using a simple string '123' as keys is acceptable for test fixtures, ensure that production configurations use strong, unique keys.
Let's verify this is indeed a test-only configuration:
✅ Verification successful
Configuration verified as test-only with appropriate test coverage
The configuration is correctly placed and used:
- Located exclusively in test fixtures directory
- Port 17010 is not used in production code
- The reusePort feature is properly tested in multiple test files including
master.test.jsandapp_worker.test.js- The configuration structure follows the test fixture pattern with appropriate test-only values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify this is a test-only configuration by checking its path # and ensuring no production code references this keys value # Test 1: Confirm this is under test directory if ! echo "test/fixtures/apps/app-listen-reusePort/config/config.default.js" | grep -q "^test/"; then echo "Warning: Configuration file not in test directory" fi # Test 2: Check for any usage of this keys value in non-test code rg -g '!test/**' "'123'"Length of output: 123
Script:
#!/bin/bash # Let's verify this more thoroughly with multiple checks # Check 1: Look for any similar config files in non-test directories fd "config.default.js" --exclude test/ # Check 2: Search for any usage of port 17010 in non-test code rg -g '!test/**' "17010" # Check 3: Look for all files in this test fixture directory to understand its scope fd . test/fixtures/apps/app-listen-reusePort/ # Check 4: Check if there are other test configurations using reusePort for comparison rg "reusePort.*true" test/Length of output: 796
test/fixtures/apps/app-listen-reusePort/app/router.js (1)
1-9: LGTM - Test fixture implementation is appropriate.The router implementation provides the necessary endpoints to validate the reusePort functionality in the test environment.
.github/workflows/nodejs.yml (1)
15-15: LGTM! Verify Node.js 23 support in the reusable workflow.Testing against Node.js 23 is valuable for catching compatibility issues early, especially for low-level socket features like
SO_REUSEPORT.Let's verify if the reusable workflow supports Node.js 23:
Note: Since Node.js 23 is currently in development, monitor CI stability and consider making it an optional test if it causes false negatives.
lib/utils/options.js (1)
15-15: Verify integration with worker and master components.Let's ensure the
reusePortoption is properly handled by the dependent components.✅ Verification successful
Based on the results, I can now generate the final response since I have sufficient information about how the
reusePortoption is integrated:Integration with worker and master components is properly implemented
The
reusePortoption is correctly handled:
- In
lib/master.js, it's properly documented with platform compatibility notes- In
lib/app_worker.js, it's properly:
- Merged with listen config:
options.reusePort = options.reusePort || listenConfig.reusePort- Used in server creation:
server.listen(listenOptions)withlistenOptionscontaining thereusePortsetting🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of reusePort option in worker and master components echo "Checking reusePort usage in app_worker.js and master.js:" rg -A 5 "reusePort" "lib/app_worker.js" "lib/master.js" echo -e "\nChecking server creation and listening logic:" ast-grep --pattern 'server.listen($$$)'Length of output: 2238
test/app_worker.test.js (1)
236-236: Verify test app configuration.Let's ensure the test app is properly configured for reusePort testing.
✅ Verification successful
Test app configuration is properly set for reusePort testing
The test app is correctly configured with:
reusePort: truein config.default.js- Custom port 17010 to avoid egg-mock's default port
- Working HTTP endpoint verified in the test implementation
- Proper cluster setup and port verification in the test
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test app configuration for reusePort setting # Test: Verify the test app has proper configuration echo "Checking test app configuration..." rg -A 5 "reusePort" "test/fixtures/apps/app-listen-reusePort/config/" # Test: Check if the app has necessary test endpoints echo "Checking test endpoints..." rg -A 5 "app.get\('/(?:reusePort|pid)'" "test/fixtures/apps/app-listen-reusePort/"Length of output: 596
Script:
#!/bin/bash # Let's check the app configuration and implementation more thoroughly # Check the complete config file content echo "Checking complete config file..." cat test/fixtures/apps/app-listen-reusePort/config/config.default.js # Check the app implementation echo -e "\nChecking app implementation..." cat test/fixtures/apps/app-listen-reusePort/app.js # Check the test file using this app echo -e "\nChecking test implementation..." cat test/app_worker.test.js | grep -A 10 "app-listen-reusePort"Length of output: 987
lib/master.js (1)
Line range hint
447-467: Verify sticky mode compatibility with reusePort.The
startMasterSocketServermethod needs to be verified for compatibility when both sticky mode and reusePort are enabled. The current implementation might need adjustments to handle connection distribution correctly in this scenario.Let's verify the implementation:
test/fixtures/apps/app-listen-reusePort/config/config.default.js
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 2.x #115 +/- ##
==========================================
- Coverage 92.52% 91.50% -1.03%
==========================================
Files 15 15
Lines 2021 2106 +85
Branches 336 346 +10
==========================================
+ Hits 1870 1927 +57
- Misses 151 179 +28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- test/app_worker.test.js (1 hunks)
🧰 Additional context used
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 23)
test/app_worker.test.js
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22)
test/app_worker.test.js
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (macos-latest, 23)
test/app_worker.test.js
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)
test/app_worker.test.js
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)
test/app_worker.test.js
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 16)
test/app_worker.test.js
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 14)
test/app_worker.test.js
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (macos-latest, 22)
test/app_worker.test.js
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (macos-latest, 20)
test/app_worker.test.js
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (macos-latest, 16)
test/app_worker.test.js
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (macos-latest, 14)
test/app_worker.test.js
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (macos-latest, 18)
test/app_worker.test.js
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 Biome
test/app_worker.test.js
[error] 235-235: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
lib/agent_worker.js (1)
Line range hint
1-58: Architecture feedback regarding reusePort integrationWhile the error handling improvements are solid, consider adding debug logs specifically for reusePort-related initialization since this file is part of the worker management system that will be affected by the new reusePort feature. This would help in troubleshooting port binding issues in distributed deployments.
Consider adding debug logs like:
try { + debug('initializing agent with reusePort: %j', options.reusePort); agent = new Agent(options); } catch (err) { consoleLogger.error(err);lib/app_worker.js (1)
43-47: Document platform limitation in README.The platform check correctly limits reusePort to Linux systems. However, this limitation should be documented in the README to set proper expectations for users.
Would you like me to help draft the documentation for the platform limitation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- lib/agent_worker.js (1 hunks)
- lib/app_worker.js (3 hunks)
- lib/utils/options.js (1 hunks)
- package.json (1 hunks)
- test/reuseport_cluster.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/utils/options.js
🧰 Additional context used
🪛 Biome
lib/app_worker.js
[error] 42-42: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🪛 GitHub Check: codecov/patch
lib/app_worker.js
[warning] 133-138: lib/app_worker.js#L133-L138
Added lines #L133 - L138 were not covered by tests
[warning] 142-143: lib/app_worker.js#L142-L143
Added lines #L142 - L143 were not covered by tests
🔇 Additional comments (7)
lib/agent_worker.js (3)
27-27: LGTM: Improved logging contextThe debug logger identifier change from 'egg-cluster' to 'egg-cluster:agent_worker' provides better context for debugging, making it easier to trace issues in distributed environments.
Line range hint
39-43: LGTM: Well-structured error handlerThe
startErrorHandlerfunction effectively centralizes error handling logic during agent initialization. It provides clear error logging and ensures proper process termination.
Line range hint
45-51: Verify error handling cleanupThe error handling flow looks good, but let's verify that the error listener cleanup is comprehensive.
Also applies to: 54-54
✅ Verification successful
Based on the results, I can now generate the final response:
Error handling and cleanup implementation is correct and comprehensive
The error handling flow is properly implemented with a clear pattern:
- The
startErrorHandleris registered usingonce()which automatically removes the listener after first trigger- The explicit
removeListener()is correctly used to clean up when agent starts successfully- No conflicting error handlers found in the agent worker context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any other error handler registrations or potential memory leaks # Test: Search for other error event handlers that might conflict rg -A 5 "\.on\('error'" || rg -A 5 "\.once\('error'" # Test: Check for proper cleanup patterns rg -A 5 "removeListener\('error'"Length of output: 1472
package.json (1)
11-11: Verify test framework compatibility with reuseport tests.Let's ensure the reuseport tests can be properly integrated with egg-bin test framework.
test/reuseport_cluster.js (1)
1-4: LGTM! Proper use of node: protocol prefix for imports.The module imports are well-structured and follow Node.js best practices by using the node: protocol prefix for core modules.
lib/app_worker.js (2)
19-21: LGTM! Improved logging granularity.The debug namespace change from 'egg-cluster' to 'egg-cluster:app_worker' provides better log segmentation, making it easier to filter worker-specific logs.
132-146: Add test coverage for hostname configuration.The code paths for hostname configuration (lines 134-136, 141-143) lack test coverage. Please add tests for:
- reusePort with hostname configuration
- reusePort without hostname configuration
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 133-138: lib/app_worker.js#L133-L138
Added lines #L133 - L138 were not covered by tests
[warning] 142-143: lib/app_worker.js#L142-L143
Added lines #L142 - L143 were not covered by tests
|
请问reuseport还没合入吗？ |
|
@dennisleung 还没，因为测试了一下性能没有变化，所以暂时还没动力合并。 |
Signed-off-by: MK (fengmk2) <[email protected]>
|
https://blog.platformatic.dev/93-faster-nextjs-in-your-kubernetes 看来是我的测试方式不对，周末改成短连接的方式再次验证一下 |
Signed-off-by: MK (fengmk2) <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: MK (fengmk2) <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: MK (fengmk2) <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR adds support for the reusePort option in server listen configuration, allowing multiple worker processes to bind to the same port with load balancing handled by the operating system. This is a Linux-focused feature (currently) that can improve performance in multi-worker scenarios.
Key Changes
- Added
reusePortoption with platform detection (Linux only in implementation) - Updated server listen logic to use reusePort when enabled
- Added test coverage for the new feature including integration tests and manual cluster test
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
lib/app_worker.js |
Implements reusePort logic with platform check and modified server.listen() call |
lib/master.js |
Adds JSDoc documentation for the reusePort option |
lib/utils/options.js |
Sets default value of reusePort to false |
lib/agent_worker.js |
Updates debug logger name for better specificity |
test/app_worker.test.js |
Adds integration test for reusePort configuration |
test/master.test.js |
Adds test for master startup with reusePort enabled |
test/reuseport_cluster.js |
New manual cluster test demonstrating reusePort functionality |
test/fixtures/apps/app-listen-reusePort/* |
Test fixture app with reusePort configuration |
package.json |
Adds manual cluster test to CI pipeline |
.github/workflows/nodejs.yml |
Adds Node.js v23 to test matrix |
README.md |
Fixes codecov badge URLs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
package.json (2)
11-11: Guardtest/reuseport_cluster.jsin CI to avoid non-Linux failures/flakes.If your CI runs on macOS/Windows, this step can fail or behave inconsistently; prefer gating to Linux (or make the test self-skip when
reusePortisn’t supported).- "ci": "npm run lint && node test/reuseport_cluster.js && npm run cov" + "ci": "npm run lint && (node -e \"process.platform==='linux' ? process.exit(0) : process.exit(42)\" || true) && ( [ \"$(uname)\" = \"Linux\" ] && node test/reuseport_cluster.js || echo 'Skipping reuseport_cluster test: not Linux' ) && npm run cov"(If your CI shell isn’t bash, do the platform guard inside
test/reuseport_cluster.jsinstead.)
11-11: Prefer running reusePort coverage via the existing test runner for consistency.Running a standalone Node script in
cibypasses the normal test reporting/coverage flow; consider moving this intoegg-bin test(e.g., an integration test file) and calling it viatest-local.
🧹 Nitpick comments (1)
lib/utils/options.js (1)
78-78: Use console.warn for consistency.Line 50 in this file uses
console.warnfor deprecation messages. For consistency, platform incompatibility warnings should also useconsole.warn.- console.log('platform %s is not support currently, set reusePort to false', os.platform()); + console.warn('[egg-cluster] platform %s does not support reusePort, disabled', os.platform());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/nodejs.yml(1 hunks).gitignore(1 hunks)README.md(2 hunks)lib/app_worker.js(3 hunks)lib/utils/mode/impl/worker_threads/app.js(1 hunks)lib/utils/options.js(3 hunks)package.json(2 hunks)test/app_worker.test.js(1 hunks)test/fixtures/apps/app-listen-reusePort/config/config.default.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (4)
- test/app_worker.test.js
- lib/app_worker.js
- .github/workflows/nodejs.yml
- test/fixtures/apps/app-listen-reusePort/config/config.default.js
🧰 Additional context used
🧬 Code graph analysis (2)
lib/utils/options.js (2)
test/reuseport_cluster.js (1)
cluster(1-1)lib/utils/mode/impl/process/app.js (2)
cluster(3-3)require(8-8)
lib/utils/mode/impl/worker_threads/app.js (2)
lib/app_worker.js (1)
options(4-4)lib/agent_worker.js (1)
options(12-12)
⏰ 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). (8)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
- GitHub Check: Node.js / Test (macos-latest, 24)
- GitHub Check: Node.js / Test (ubuntu-latest, 24)
- GitHub Check: Node.js / Test (macos-latest, 22)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (macos-latest, 20)
- GitHub Check: Node.js / Test (macos-latest, 18)
- GitHub Check: Node.js / Test (ubuntu-latest, 18)
🔇 Additional comments (4)
package.json (1)
19-31: Repo/bugs/homepage URL updates look consistent.These metadata links now point to
eggjs/cluster, matching the PR context.lib/utils/mode/impl/worker_threads/app.js (1)
158-176: LGTM! The conditional forking logic correctly implements reusePort support.The implementation properly branches based on
reusePort:
- When enabled: spawns N workers with identical options, allowing SO_REUSEPORT to distribute connections
- When disabled: preserves the per-port worker allocation strategy
lib/utils/options.js (1)
74-84: The reusePort handling logic is sound.The implementation correctly:
- Disables
reusePorton non-Linux platforms- Sets
cluster.schedulingPolicy = cluster.SCHED_NONEon Linux to delegate load balancing to the kernel when using SO_REUSEPORTREADME.md (1)
56-56: No action required. The Node.js version requirement forreusePortis accurate. Theserver.listen({ reusePort })option was indeed added in Node.js v22.12.0, making the README.md documentation correct.Likely an incorrect or invalid review comment.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
lib/app_worker.js (2)
136-136: CRITICAL: Remove trailing whitespace causing CI failures.Line 136 contains trailing whitespace that is causing all CI checks to fail. This must be fixed before the PR can be merged.
Apply this diff:
- // https://nodejs.org/api/net.html#serverlistenoptions-callback + // https://nodejs.org/api/net.html#serverlistenoptions-callback(Remove the trailing space(s) at the end of the comment line)
43-43: Add type validation for reusePort option.While port validation exists (lines 129-133), there's no validation for the reusePort option. This could lead to unexpected behavior if a non-boolean value is passed.
Apply this diff to add type validation before using reusePort:
let reusePort = options.reusePort = options.reusePort || listenConfig.reusePort; +if (reusePort && typeof reusePort !== 'boolean') { + consoleLogger.error('[app_worker] reusePort should be boolean, but got %s(%s)', reusePort, typeof reusePort); + exitProcess(); + return; +} if (reusePort && os.platform() !== 'linux') {lib/utils/mode/impl/worker_threads/app.js (1)
179-181: Validate port is defined when ports array is empty.When
reusePortisfalseandportsarray is empty,this.options.portis pushed into the array. However,this.options.portcan beundefined, causing workers to attempt listening on an undefined port.Apply this diff to add validation:
const ports = this.options.ports; if (!ports.length) { + if (!this.options.port) { + throw new Error('options.port must be specified when ports array is empty'); + } ports.push(this.options.port);
🧹 Nitpick comments (1)
lib/app_worker.js (1)
44-49: Expand reusePort platform support beyond Linux.The current implementation restricts reusePort to Linux only, but Node.js documentation indicates reusePort is available on Linux 3.9+, DragonFlyBSD 3.6+, FreeBSD 12.0+, Solaris 11.4, and AIX 7.2.5+. Either expand platform support to match Node.js capabilities or document why only Linux is currently supported.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/app_worker.js(3 hunks)lib/utils/mode/impl/worker_threads/app.js(3 hunks)lib/utils/options.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/utils/options.js
🧰 Additional context used
🧬 Code graph analysis (2)
lib/utils/mode/impl/worker_threads/app.js (4)
lib/app_worker.js (2)
options(4-4)AppWorker(12-12)lib/utils/messenger.js (1)
workerThreads(4-4)lib/utils/mode/impl/worker_threads/agent.js (1)
workerThreads(3-3)test/fixtures/apps/agent-worker-threads/agent.js (1)
workerThreads(3-3)
lib/app_worker.js (4)
lib/utils/options.js (2)
os(3-3)fs(4-4)lib/master.js (3)
os(3-3)require(10-10)fs(7-7)test/options.test.js (1)
os(3-3)test/fixtures/apps/cluster_mod_sticky/app.js (1)
server(8-8)
⏰ 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). (8)
- GitHub Check: Node.js / Test (macos-latest, 24)
- GitHub Check: Node.js / Test (macos-latest, 18)
- GitHub Check: Node.js / Test (ubuntu-latest, 24)
- GitHub Check: Node.js / Test (macos-latest, 22)
- GitHub Check: Node.js / Test (ubuntu-latest, 18)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (macos-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
🔇 Additional comments (2)
lib/app_worker.js (1)
19-23: LGTM!The os module import is correctly added to support platform detection for reusePort, and the debug namespace update aligns with the module naming.
lib/utils/mode/impl/worker_threads/app.js (1)
194-201: LGTM!The kill method correctly sets
disableReforkbefore terminating workers, preventing unwanted reforks. Listener cleanup is appropriate to avoid memory leaks.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/utils/mode/impl/worker_threads/app.js (1)
174-195: Good:reusePortnow fails fast withoutoptions.port.
Nice preflight check (Line 175-177); prevents forking workers that can’t listen.Still missing: guard against undefined port when
reusePortis false andportsis empty.
This keeps the old footgun:ports.push(this.options.port)(Line 185-187) can still pushundefinedand later fork workers with invalidport.} else { const ports = this.options.ports; if (!ports.length) { + if (!this.options.port) { + throw new Error('options.port must be specified when reusePort is disabled and ports is empty'); + } ports.push(this.options.port); } this.options.workers = ports.length; let i = 0;
🧹 Nitpick comments (1)
lib/utils/mode/impl/worker_threads/app.js (1)
200-206:kill()should awaitterminate()and clear internal state.
Worker#terminate()returns a Promise; not awaiting can cause callers to proceed while threads are still alive. Also consider clearing#appWorkersonce termination is initiated/completed.async kill() { - for (const appWorker of this.#appWorkers) { + const terminating = []; + for (const appWorker of this.#appWorkers) { this.log('[master] kill app_worker#%s (tid:%s) (worker_threads) by worker.terminate()', appWorker.id, appWorker.workerId); appWorker.disableRefork = true; appWorker.instance.removeAllListeners(); - appWorker.instance.terminate(); + terminating.push(appWorker.instance.terminate()); } + await Promise.allSettled(terminating); + this.#appWorkers = []; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/utils/mode/impl/worker_threads/app.js(3 hunks)test/reuseport_cluster.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/reuseport_cluster.js
⏰ 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). (11)
- GitHub Check: Node.js / Test (ubuntu-latest, 24)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
- GitHub Check: Node.js / Test (macos-latest, 14)
- GitHub Check: Node.js / Test (macos-latest, 20)
- GitHub Check: Node.js / Test (macos-latest, 24)
- GitHub Check: Node.js / Test (macos-latest, 18)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (macos-latest, 22)
- GitHub Check: Node.js / Test (ubuntu-latest, 18)
- GitHub Check: Node.js / Test (ubuntu-latest, 14)
- GitHub Check: Node.js / Test (ubuntu-latest, 16)
🔇 Additional comments (1)
lib/utils/mode/impl/worker_threads/app.js (1)
74-82: Rename to#appWorkersis clear and consistent with “app worker” semantics.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/app_worker.test.js (1)
235-262: Removeit.onlyand comment out debug call.This test still uses
it.onlywhich blocks all other tests from running, causing CI failures. Theapp.debug()call should also be commented out for cleaner test output.Apply this diff:
- it.only('should set reusePort=true in config', async () => { + it('should set reusePort=true in config', async () => { app = utils.cluster('apps/app-listen-reusePort'); - app.debug(); + // app.debug(); await app.ready();lib/app_worker.js (1)
43-53: Add DragonFlyBSD to supported platforms list.The comment on line 46 mentions "DragonFlyBSD 3.6+" but the
supportedPlatformsarray is missing'dragonfly'. Node.js returns'dragonfly'fromos.platform()on DragonFlyBSD systems.Apply this diff:
// This option is available only on some platforms, such as Linux 3.9+, DragonFlyBSD 3.6+, FreeBSD 12.0+, Solaris 11.4, and AIX 7.2.5+. -const supportedPlatforms = [ 'linux', 'freebsd', 'sunos', 'aix' ]; +const supportedPlatforms = [ 'linux', 'freebsd', 'sunos', 'aix', 'dragonfly' ];
🧹 Nitpick comments (1)
lib/app_worker.js (1)
146-153: Use%jformatter for array in debug statement.Line 151 uses
%sfor theargsarray, which won't display the array structure properly. For consistency with line 144 and better debug output, use%j.Apply this diff:
} else { const args = [ port ]; if (listenConfig.hostname) { args.push(listenConfig.hostname); } - debug('listen options %s', args); + debug('listen options %j', args); server.listen(...args); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/app_worker.js(3 hunks)test/app_worker.test.js(1 hunks)test/master.test.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/app_worker.js (4)
lib/utils/options.js (2)
os(3-3)fs(4-4)lib/master.js (4)
os(3-3)require(10-10)fs(7-7)ConsoleLogger(11-11)test/options.test.js (1)
os(3-3)lib/agent_worker.js (3)
debug(27-27)ConsoleLogger(28-28)options(12-12)
test/master.test.js (1)
test/fixtures/apps/script-start/start-server.js (2)
app(5-5)utils(2-2)
🪛 GitHub Check: Node.js / Test (macos-latest, 16)
test/app_worker.test.js
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
test/master.test.js
[warning] 32-32:
it.only not permitted
[warning] 32-32:
it.only not permitted
🪛 GitHub Check: Node.js / Test (macos-latest, 18)
test/app_worker.test.js
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
test/master.test.js
[warning] 32-32:
it.only not permitted
[warning] 32-32:
it.only not permitted
🪛 GitHub Check: Node.js / Test (macos-latest, 22)
test/app_worker.test.js
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
test/master.test.js
[warning] 32-32:
it.only not permitted
[warning] 32-32:
it.only not permitted
🪛 GitHub Check: Node.js / Test (macos-latest, 24)
test/app_worker.test.js
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
test/master.test.js
[warning] 32-32:
it.only not permitted
[warning] 32-32:
it.only not permitted
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 14)
test/app_worker.test.js
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
test/master.test.js
[warning] 32-32:
it.only not permitted
[warning] 32-32:
it.only not permitted
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 16)
test/app_worker.test.js
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
test/master.test.js
[warning] 32-32:
it.only not permitted
[warning] 32-32:
it.only not permitted
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)
test/app_worker.test.js
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
test/master.test.js
[warning] 32-32:
it.only not permitted
[warning] 32-32:
it.only not permitted
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)
test/app_worker.test.js
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
test/master.test.js
[warning] 32-32:
it.only not permitted
[warning] 32-32:
it.only not permitted
⏰ 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). (4)
- GitHub Check: Node.js / Test (macos-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 24)
- GitHub Check: Node.js / Test (macos-latest, 14)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
🔇 Additional comments (1)
lib/app_worker.js (1)
138-145: LGTM - reusePort listen implementation is well-structured.The implementation correctly builds the listen options with
portandreusePort, and properly includeshostwhenlistenConfig.hostnameis set. This aligns well with the Node.jsserver.listen(options)API.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/utils/mode/impl/process/app.js (1)
129-142: Fix the action name mismatch in the message handler and add deduplication to prevent doubleapp-start.The code has two potential paths for emitting
app-start: one viacluster.on('listening')and another viacluster.on('message'). Currently, the message handler checks foraction !== 'reuse-port-listening', but the worker sendsaction: 'listening'. If the action name is corrected, both handlers could fire simultaneously whenreusePortis enabled, causing duplicateapp-startemissions. Either fix the action name and implement deduplication (viaSetkeyed byworker.idorpid), or ensure only one code path handles the listening event based on thereusePortflag.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/app_worker.js(4 hunks)lib/utils/mode/impl/process/app.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/app_worker.js
⏰ 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). (3)
- GitHub Check: Node.js / Test (macos-latest, 18)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
- GitHub Check: Node.js / Test (ubuntu-latest, 24)
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for the reusePort option, enabling multiple sockets to bind to the same port for improved load distribution. Key changes include adding the reusePort option to the cluster configuration and documentation, implementing platform-specific reusePort logic in app_worker.js for server listening, and adapting worker forking and listening event handling in worker_threads/app.js and process/app.js to accommodate this new feature. The debuglog namespaces were updated, pnpm-lock.yaml was added to .gitignore, and package metadata was revised. New test cases were added to verify the reusePort functionality. Review comments highlighted the omission of 'dragonfly' from the supportedPlatforms array in app_worker.js, suggested refactoring a do-while loop to a for loop in worker_threads/app.js for better readability, and recommended replacing the deprecated cluster.isMaster with cluster.isPrimary in a test script.
| // https://github.com/nodejs/node/blob/main/node.gypi#L310 | ||
| // https://docs.python.org/3/library/sys.html#sys.platform | ||
| // This option is available only on some platforms, such as Linux 3.9+, DragonFlyBSD 3.6+, FreeBSD 12.0+, Solaris 11.4, and AIX 7.2.5+. | ||
| const supportedPlatforms = [ 'linux', 'freebsd', 'sunos', 'aix' ]; |
There was a problem hiding this comment.
The supportedPlatforms array is missing support for DragonFlyBSD. The README.md mentions it as a supported platform, but os.platform() returns 'dragonfly', which is not in this list. This will prevent reusePort from being enabled on that platform.
| const supportedPlatforms = [ 'linux', 'freebsd', 'sunos', 'aix' ]; | |
| const supportedPlatforms = [ 'linux', 'freebsd', 'sunos', 'aix', 'dragonfly' ]; |
| let i = 0; | ||
| do { | ||
| const options = Object.assign({}, this.options, { port: ports[i] }); | ||
| const argv = [ JSON.stringify(options) ]; | ||
| this.#forkSingle(this.getAppWorkerFile(), { argv }, ++i); | ||
| } while (i < ports.length); |
There was a problem hiding this comment.
The do-while loop used here is a bit unconventional and less readable than a standard for loop for this kind of iteration. Using a for loop would make the intent clearer and improve maintainability.
for (let i = 0; i < ports.length; i++) {
const options = Object.assign({}, this.options, { port: ports[i] });
const argv = [ JSON.stringify(options) ];
this.#forkSingle(this.getAppWorkerFile(), { argv }, i + 1);
}| }); | ||
| } | ||
|
|
||
| if (cluster.isPrimary || cluster.isMaster) { |
There was a problem hiding this comment.
cluster.isMaster has been deprecated since Node.js v16.0.0. It's better to use cluster.isPrimary for forward compatibility and to adhere to modern Node.js practices.
| if (cluster.isPrimary || cluster.isMaster) { | |
| if (cluster.isPrimary) { |
|
🎉 This PR is included in version 2.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
closes eggjs/egg#5365
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation
Other
✏️ Tip: You can customize this high-level summary in your review settings.