feat: support reusePort on server listen #115
Conversation
WalkthroughThe changes in this pull request involve the removal of the pull request template, updates to the Node.js CI workflow to include version 23, and modifications to the Changes
Assessment against linked issues
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: 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:
| 'use strict'; | ||
|
|
There was a problem hiding this comment.
Remove redundant 'use strict' directive.
The 'use strict' directive is unnecessary in ES modules as they are automatically in strict mode.
Apply this diff:
-'use strict';
-📝 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.
| 'use strict'; |
🧰 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)
lib/app_worker.js
Outdated
| @@ -35,6 +35,7 @@ const clusterConfig = app.config.cluster || /* istanbul ignore next */ {}; | |||
| const listenConfig = clusterConfig.listen || /* istanbul ignore next */ {}; | |||
| const httpsOptions = Object.assign({}, clusterConfig.https, options.https); | |||
| const port = options.port = options.port || listenConfig.port; | |||
| const reusePort = options.reusePort = options.reusePort || listenConfig.reusePort; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve code clarity by avoiding assignment in expression.
The current initialization pattern can make the code harder to read. Consider making the assignments more explicit.
-const reusePort = options.reusePort = options.reusePort || listenConfig.reusePort;
+options.reusePort = options.reusePort || listenConfig.reusePort;
+const reusePort = options.reusePort;📝 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.
| const reusePort = options.reusePort = options.reusePort || listenConfig.reusePort; | |
| options.reusePort = options.reusePort || listenConfig.reusePort; | |
| const reusePort = options.reusePort; |
🧰 Tools
🪛 Biome
[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)
| if (reusePort) { | ||
| const listenOptions = { port, reusePort }; | ||
| if (listenConfig.hostname) { | ||
| listenOptions.host = listenConfig.hostname; | ||
| } | ||
| debug('listen options %s', listenOptions); | ||
| server.listen(listenOptions); | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation for reusePort option.
While port validation exists, there's no validation for the reusePort option. Consider adding type checking to ensure robust error handling.
if (reusePort) {
+ if (typeof reusePort !== 'boolean') {
+ consoleLogger.error('[app_worker] reusePort should be boolean, but got %s(%s)', reusePort, typeof reusePort);
+ exitProcess();
+ return;
+ }
const listenOptions = { port, reusePort };📝 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.
| if (reusePort) { | |
| const listenOptions = { port, reusePort }; | |
| if (listenConfig.hostname) { | |
| listenOptions.host = listenConfig.hostname; | |
| } | |
| debug('listen options %s', listenOptions); | |
| server.listen(listenOptions); | |
| } else { | |
| if (reusePort) { | |
| if (typeof reusePort !== 'boolean') { | |
| consoleLogger.error('[app_worker] reusePort should be boolean, but got %s(%s)', reusePort, typeof reusePort); | |
| exitProcess(); | |
| return; | |
| } | |
| const listenOptions = { port, reusePort }; | |
| if (listenConfig.hostname) { | |
| listenOptions.host = listenConfig.hostname; | |
| } | |
| debug('listen options %s', listenOptions); | |
| server.listen(listenOptions); | |
| } else { |
| if (reusePort) { | ||
| const listenOptions = { port, reusePort }; | ||
| if (listenConfig.hostname) { | ||
| listenOptions.host = listenConfig.hostname; | ||
| } | ||
| debug('listen options %s', listenOptions); | ||
| server.listen(listenOptions); | ||
| } else { | ||
| const args = [ port ]; | ||
| if (listenConfig.hostname) { | ||
| args.push(listenConfig.hostname); | ||
| } | ||
| debug('listen options %s', args); | ||
| server.listen(...args); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Additional test coverage needed for reusePort scenarios
The current test suite only verifies basic functionality of reusePort with a single worker. Missing test coverage for:
- Multiple workers binding to the same port with reusePort enabled
- Error handling when some workers have reusePort enabled and others don't
- Load balancing behavior across multiple workers with reusePort
Suggested test additions:
- Test with multiple workers (2+) all using reusePort
- Test error scenarios with mixed reusePort configurations
- Test connection distribution across workers
🔗 Analysis chain
Verify reusePort behavior under different scenarios.
The implementation looks good and aligns with the PR objectives. Consider testing these scenarios:
- Multiple workers binding to the same port
- Performance impact with and without reusePort
- Behavior when some workers have reusePort enabled and others don't
Let me check the test coverage in detail to verify if all scenarios are properly tested.
Let me check if there are tests covering the multiple workers scenario and error cases.
Let me check the actual test implementation for the reusePort configuration to see how it's being tested with multiple workers.
Let me check the test fixture implementation to understand how reusePort is configured.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check test coverage for reusePort scenarios
rg -l "reusePort.*true" test/
Length of output: 140
Script:
#!/bin/bash
# Check test implementations for reusePort
rg -A 10 "reusePort.*true" test/
Length of output: 1663
Script:
#!/bin/bash
# Check for worker-related tests with reusePort
rg -B 5 -A 15 "workers.*=.*[2-9]" test/
Length of output: 2283
Script:
#!/bin/bash
# Check the full test implementation for reusePort
cat test/app_worker.test.js
Length of output: 9691
Script:
#!/bin/bash
# Check the test fixture implementation
cat test/fixtures/apps/app-listen-reusePort/config/config.default.js
Length of output: 203
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #115 +/- ##
===========================================
- Coverage 92.52% 66.29% -26.24%
===========================================
Files 15 15
Lines 2021 2041 +20
Branches 336 190 -146
===========================================
- Hits 1870 1353 -517
- Misses 151 688 +537 ☔ View full report in Codecov by Sentry. |
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)
| @@ -232,6 +232,35 @@ | |||
| .expect(200); | |||
| }); | |||
|
|
|||
| it.only('should set reusePort=true in config', async () => { | |||
There was a problem hiding this comment.
Remove .only from the test case.
The use of it.only prevents other tests from running. This is typically used during development for debugging purposes but should be removed before merging.
Apply this diff to fix the issue:
- it.only('should set reusePort=true in config', async () => {
+ it('should set reusePort=true in config', async () => {📝 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.
| it.only('should set reusePort=true in config', async () => { | |
| it('should set reusePort=true in config', async () => { |
🧰 Tools
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 23)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (macos-latest, 23)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 16)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 14)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (macos-latest, 22)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (macos-latest, 20)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (macos-latest, 16)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (macos-latest, 14)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (macos-latest, 18)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 Biome
[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)
| it.only('should set reusePort=true in config', async () => { | ||
| app = utils.cluster('apps/app-listen-reusePort'); | ||
| // app.debug(); | ||
| await app.ready(); | ||
|
|
||
| app.expect('code', 0); | ||
| app.expect('stdout', /egg started on http:\/\/127.0.0.1:17010/); | ||
|
|
||
| await request('http://0.0.0.0:17010') | ||
| .get('/') | ||
| .expect('done') | ||
| .expect(200); | ||
|
|
||
| await request('http://127.0.0.1:17010') | ||
| .get('/') | ||
| .expect('done') | ||
| .expect(200); | ||
|
|
||
| await request('http://localhost:17010') | ||
| .get('/') | ||
| .expect('done') | ||
| .expect(200); | ||
|
|
||
| await request('http://127.0.0.1:17010') | ||
| .get('/port') | ||
| .expect('17010') | ||
| .expect(200); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for reusePort functionality.
While the test verifies basic HTTP functionality, it doesn't specifically verify that the SO_REUSEPORT option is properly set. Consider adding assertions to validate the socket option.
Here's a suggested enhancement to verify reusePort is actually set:
it('should set reusePort=true in config', async () => {
app = utils.cluster('apps/app-listen-reusePort');
- // app.debug();
await app.ready();
app.expect('code', 0);
app.expect('stdout', /egg started on http:\/\/127.0.0.1:17010/);
+ // Verify reusePort is set in the worker process
+ await app.httpRequest()
+ .get('/get-socket-options')
+ .expect(res => {
+ assert(res.body.reusePort === true, 'Expected SO_REUSEPORT to be enabled');
+ })
+ .expect(200);
await request('http://0.0.0.0:17010')
.get('/')This requires adding a new endpoint in your test fixture that exposes the socket options. I can help implement this if needed.
Would you like me to provide the implementation for the socket options endpoint in the test fixture?
Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 23)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (macos-latest, 23)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 16)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 14)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (macos-latest, 22)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (macos-latest, 20)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (macos-latest, 16)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (macos-latest, 14)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 GitHub Check: Node.js / Test (macos-latest, 18)
[warning] 235-235:
it.only not permitted
[warning] 235-235:
it.only not permitted
🪛 Biome
[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
| @@ -8,7 +8,7 @@ | |||
| "test": "npm run lint -- --fix && npm run test-local", | |||
| "test-local": "egg-bin test --ts false", | |||
| "cov": "egg-bin cov --prerequire --timeout 100000 --ts false", | |||
| "ci": "npm run lint && npm run cov", | |||
| "ci": "npm run lint && node test/reuseport_cluster.js && npm run cov", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider integrating reuseport test into the main test suite.
While adding the test to CI is good, running it directly with Node bypasses the test framework. Consider moving this test into the main test suite to:
- Ensure consistent test reporting
- Include results in coverage metrics
- Leverage the test framework's features
Apply this diff to integrate with the test framework:
- "ci": "npm run lint && node test/reuseport_cluster.js && npm run cov",
+ "ci": "npm run lint && npm run test-local && npm run cov",Then move the reuseport cluster test logic into a proper test file that integrates with your test framework.
Committable suggestion was skipped due to low confidence.
| if (cluster.isPrimary) { | ||
| console.log(`Primary ${process.pid} is running`); | ||
|
|
||
| // Fork workers. | ||
| for (let i = 0; i < numCPUs; i++) { | ||
| cluster.fork(); | ||
| } | ||
|
|
||
| cluster.on('exit', (worker, code, signal) => { | ||
| console.log(`worker ${worker.process.pid} died, code: ${code}, signal: ${signal}`); | ||
| }); | ||
|
|
||
| setTimeout(() => { | ||
| for (let i = 0; i < 20; i++) { | ||
| request(i); | ||
| } | ||
| }, 2000); | ||
| setTimeout(() => { | ||
| process.exit(0); | ||
| }, 5000); | ||
| } else { |
There was a problem hiding this comment.
Improve primary process test reliability and verification.
The current implementation has potential race conditions and lacks proper verification of the reusePort functionality.
Consider these improvements:
if (cluster.isPrimary) {
console.log(`Primary ${process.pid} is running`);
+ const workers = new Set();
// Fork workers.
for (let i = 0; i < numCPUs; i++) {
- cluster.fork();
+ const worker = cluster.fork();
+ workers.add(worker);
}
+ // Track worker readiness
+ let readyWorkers = 0;
+ cluster.on('message', (worker, message) => {
+ if (message === 'ready') {
+ readyWorkers++;
+ if (readyWorkers === numCPUs) {
+ runTests();
+ }
+ }
+ });
+
cluster.on('exit', (worker, code, signal) => {
console.log(`worker ${worker.process.pid} died, code: ${code}, signal: ${signal}`);
+ workers.delete(worker);
+ if (workers.size === 0) {
+ process.exit(process.exitCode || 0);
+ }
});
- setTimeout(() => {
+ function runTests() {
+ const results = new Map();
for (let i = 0; i < 20; i++) {
- request(i);
+ request(i, (workerId) => {
+ results.set(i, workerId);
+ if (results.size === 20) {
+ verifyDistribution(results);
+ }
+ });
}
- }, 2000);
- setTimeout(() => {
- process.exit(0);
- }, 5000);
+ }
+
+ function verifyDistribution(results) {
+ const workerHits = new Map();
+ for (const workerId of results.values()) {
+ workerHits.set(workerId, (workerHits.get(workerId) || 0) + 1);
+ }
+ // Verify that requests were distributed across workers
+ if (workerHits.size < numCPUs) {
+ console.error('Requests were not distributed across all workers');
+ process.exitCode = 1;
+ }
+ process.exit(process.exitCode || 0);
+ }Changes:
- Added worker readiness tracking
- Implemented request distribution verification
- Improved process exit handling
Committable suggestion was skipped due to low confidence.
| } else { | ||
| // Workers can share any TCP connection | ||
| // In this case it is an HTTP server | ||
| http.createServer((req, res) => { | ||
| res.writeHead(200); | ||
| res.end('hello world\n'); | ||
| }).listen({ | ||
| port: 17001, | ||
| reusePort: true, | ||
| }); | ||
|
|
||
| console.log(`Worker ${process.pid} started`); | ||
| } |
There was a problem hiding this comment.
Enhance worker process reliability and error handling.
The worker process needs better error handling and lifecycle management.
Apply these improvements:
} else {
// Workers can share any TCP connection
// In this case it is an HTTP server
- http.createServer((req, res) => {
+ const server = http.createServer((req, res) => {
res.writeHead(200);
+ res.write(`worker ${process.pid}\n`);
res.end('hello world\n');
}).listen({
port: 17001,
reusePort: true,
+ }, () => {
+ console.log(`Worker ${process.pid} started`);
+ // Notify primary that we're ready
+ process.send('ready');
});
- console.log(`Worker ${process.pid} started`);
+ // Handle errors
+ server.on('error', (err) => {
+ console.error(`Worker ${process.pid} server error:`, err);
+ process.exit(1);
+ });
+
+ // Graceful shutdown
+ process.on('SIGTERM', () => {
+ server.close(() => {
+ process.exit(0);
+ });
+ });
}Changes:
- Added server startup error handling
- Included worker ID in response for distribution verification
- Added graceful shutdown handling
- Implemented worker readiness notification
📝 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.
| } else { | |
| // Workers can share any TCP connection | |
| // In this case it is an HTTP server | |
| http.createServer((req, res) => { | |
| res.writeHead(200); | |
| res.end('hello world\n'); | |
| }).listen({ | |
| port: 17001, | |
| reusePort: true, | |
| }); | |
| console.log(`Worker ${process.pid} started`); | |
| } | |
| } else { | |
| // Workers can share any TCP connection | |
| // In this case it is an HTTP server | |
| const server = http.createServer((req, res) => { | |
| res.writeHead(200); | |
| res.write(`worker ${process.pid}\n`); | |
| res.end('hello world\n'); | |
| }).listen({ | |
| port: 17001, | |
| reusePort: true, | |
| }, () => { | |
| console.log(`Worker ${process.pid} started`); | |
| // Notify primary that we're ready | |
| process.send('ready'); | |
| }); | |
| // Handle errors | |
| server.on('error', (err) => { | |
| console.error(`Worker ${process.pid} server error:`, err); | |
| process.exit(1); | |
| }); | |
| // Graceful shutdown | |
| process.on('SIGTERM', () => { | |
| server.close(() => { | |
| process.exit(0); | |
| }); | |
| }); | |
| } |
| function request(index) { | ||
| http.get('http://localhost:17001/', res => { | ||
| const { statusCode } = res; | ||
| console.log(index, res.statusCode, res.headers); | ||
| let error; | ||
| // Any 2xx status code signals a successful response but | ||
| // here we're only checking for 200. | ||
| if (statusCode !== 200) { | ||
| error = new Error('Request Failed.\n' + | ||
| `Status Code: ${statusCode}`); | ||
| } | ||
| if (error) { | ||
| console.error(error.message); | ||
| // Consume response data to free up memory | ||
| res.resume(); | ||
| return; | ||
| } | ||
| res.setEncoding('utf8'); | ||
| let rawData = ''; | ||
| res.on('data', chunk => { rawData += chunk; }); | ||
| res.on('end', () => { | ||
| try { | ||
| console.log(rawData); | ||
| } catch (e) { | ||
| console.error(e.message); | ||
| } | ||
| }); | ||
| }).on('error', e => { | ||
| console.error(`Got error: ${e.stack}`); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance request helper function reliability.
While the function handles basic error cases, there are several improvements that could make the tests more reliable:
Consider these enhancements:
function request(index) {
- http.get('http://localhost:17001/', res => {
+ const req = http.get('http://localhost:17001/', res => {
const { statusCode } = res;
console.log(index, res.statusCode, res.headers);
let error;
// Any 2xx status code signals a successful response but
// here we're only checking for 200.
if (statusCode !== 200) {
error = new Error('Request Failed.\n' +
`Status Code: ${statusCode}`);
}
if (error) {
console.error(error.message);
// Consume response data to free up memory
res.resume();
+ process.exitCode = 1;
return;
}
res.setEncoding('utf8');
let rawData = '';
res.on('data', chunk => { rawData += chunk; });
res.on('end', () => {
try {
console.log(rawData);
} catch (e) {
console.error(e.message);
+ process.exitCode = 1;
}
});
}).on('error', e => {
console.error(`Got error: ${e.stack}`);
+ process.exitCode = 1;
});
+
+ // Add timeout to prevent hanging tests
+ req.setTimeout(5000, () => {
+ console.error(`Request ${index} timed out`);
+ req.destroy();
+ process.exitCode = 1;
+ });
}Changes:
- Added request timeout to prevent hanging tests
- Set non-zero exit code on errors to fail the test
- Stored request object for proper cleanup
📝 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.
| function request(index) { | |
| http.get('http://localhost:17001/', res => { | |
| const { statusCode } = res; | |
| console.log(index, res.statusCode, res.headers); | |
| let error; | |
| // Any 2xx status code signals a successful response but | |
| // here we're only checking for 200. | |
| if (statusCode !== 200) { | |
| error = new Error('Request Failed.\n' + | |
| `Status Code: ${statusCode}`); | |
| } | |
| if (error) { | |
| console.error(error.message); | |
| // Consume response data to free up memory | |
| res.resume(); | |
| return; | |
| } | |
| res.setEncoding('utf8'); | |
| let rawData = ''; | |
| res.on('data', chunk => { rawData += chunk; }); | |
| res.on('end', () => { | |
| try { | |
| console.log(rawData); | |
| } catch (e) { | |
| console.error(e.message); | |
| } | |
| }); | |
| }).on('error', e => { | |
| console.error(`Got error: ${e.stack}`); | |
| }); | |
| } | |
| function request(index) { | |
| const req = http.get('http://localhost:17001/', res => { | |
| const { statusCode } = res; | |
| console.log(index, res.statusCode, res.headers); | |
| let error; | |
| // Any 2xx status code signals a successful response but | |
| // here we're only checking for 200. | |
| if (statusCode !== 200) { | |
| error = new Error('Request Failed.\n' + | |
| `Status Code: ${statusCode}`); | |
| } | |
| if (error) { | |
| console.error(error.message); | |
| // Consume response data to free up memory | |
| res.resume(); | |
| process.exitCode = 1; | |
| return; | |
| } | |
| res.setEncoding('utf8'); | |
| let rawData = ''; | |
| res.on('data', chunk => { rawData += chunk; }); | |
| res.on('end', () => { | |
| try { | |
| console.log(rawData); | |
| } catch (e) { | |
| console.error(e.message); | |
| process.exitCode = 1; | |
| } | |
| }); | |
| }).on('error', e => { | |
| console.error(`Got error: ${e.stack}`); | |
| process.exitCode = 1; | |
| }); | |
| // Add timeout to prevent hanging tests | |
| req.setTimeout(5000, () => { | |
| console.error(`Request ${index} timed out`); | |
| req.destroy(); | |
| process.exitCode = 1; | |
| }); | |
| } |
| if (reusePort) { | ||
| const listenOptions = { port, reusePort }; |
There was a problem hiding this comment.
Add type validation for reusePort option.
While port validation exists (line 127-131), there's no validation for the reusePort option. Consider adding type checking to ensure robust error handling.
if (reusePort) {
+ if (typeof reusePort !== 'boolean') {
+ consoleLogger.error('[app_worker] reusePort should be boolean, but got %s(%s)', reusePort, typeof reusePort);
+ exitProcess();
+ return;
+ }
const listenOptions = { port, reusePort };📝 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.
| if (reusePort) { | |
| const listenOptions = { port, reusePort }; | |
| if (reusePort) { | |
| if (typeof reusePort !== 'boolean') { | |
| consoleLogger.error('[app_worker] reusePort should be boolean, but got %s(%s)', reusePort, typeof reusePort); | |
| exitProcess(); | |
| return; | |
| } | |
| const listenOptions = { port, reusePort }; |
closes eggjs/egg#5365
Summary by CodeRabbit
Release Notes
New Features
reusePortoption for server configuration, allowing multiple sockets to bind to the same port.reusePort.Bug Fixes
reusePortconfiguration to ensure proper application startup.Chores