-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: support reusePort on server listen #115
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -16,8 +16,9 @@ | |||||||||||||||||||||||||||||||||||||||||||
AppWorker = require('./utils/mode/impl/process/app').AppWorker; | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
const os = require('os'); | ||||||||||||||||||||||||||||||||||||||||||||
const fs = require('fs'); | ||||||||||||||||||||||||||||||||||||||||||||
const debug = require('util').debuglog('egg-cluster'); | ||||||||||||||||||||||||||||||||||||||||||||
const debug = require('util').debuglog('egg-cluster:app_worker'); | ||||||||||||||||||||||||||||||||||||||||||||
const ConsoleLogger = require('egg-logger').EggConsoleLogger; | ||||||||||||||||||||||||||||||||||||||||||||
const consoleLogger = new ConsoleLogger({ | ||||||||||||||||||||||||||||||||||||||||||||
level: process.env.EGG_APP_WORKER_LOGGER_LEVEL, | ||||||||||||||||||||||||||||||||||||||||||||
|
@@ -38,6 +39,13 @@ | |||||||||||||||||||||||||||||||||||||||||||
const debugPort = options.debugPort; | ||||||||||||||||||||||||||||||||||||||||||||
const protocol = (httpsOptions.key && httpsOptions.cert) ? 'https' : 'http'; | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
let reusePort = options.reusePort = options.reusePort || listenConfig.reusePort; | ||||||||||||||||||||||||||||||||||||||||||||
if (reusePort && os.platform() !== 'linux') { | ||||||||||||||||||||||||||||||||||||||||||||
// Currently only linux is supported | ||||||||||||||||||||||||||||||||||||||||||||
reusePort = false; | ||||||||||||||||||||||||||||||||||||||||||||
debug('platform %s is not support currently, set reusePort to false', os.platform()); | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
AppWorker.send({ | ||||||||||||||||||||||||||||||||||||||||||||
to: 'master', | ||||||||||||||||||||||||||||||||||||||||||||
action: 'realport', | ||||||||||||||||||||||||||||||||||||||||||||
|
@@ -121,10 +129,21 @@ | |||||||||||||||||||||||||||||||||||||||||||
exitProcess(); | ||||||||||||||||||||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
const args = [ port ]; | ||||||||||||||||||||||||||||||||||||||||||||
if (listenConfig.hostname) args.push(listenConfig.hostname); | ||||||||||||||||||||||||||||||||||||||||||||
debug('listen options %s', args); | ||||||||||||||||||||||||||||||||||||||||||||
server.listen(...args); | ||||||||||||||||||||||||||||||||||||||||||||
if (reusePort) { | ||||||||||||||||||||||||||||||||||||||||||||
const listenOptions = { port, reusePort }; | ||||||||||||||||||||||||||||||||||||||||||||
if (listenConfig.hostname) { | ||||||||||||||||||||||||||||||||||||||||||||
listenOptions.host = listenConfig.hostname; | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
debug('listen options %s', listenOptions); | ||||||||||||||||||||||||||||||||||||||||||||
server.listen(listenOptions); | ||||||||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+132
to
+139
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
const args = [ port ]; | ||||||||||||||||||||||||||||||||||||||||||||
if (listenConfig.hostname) { | ||||||||||||||||||||||||||||||||||||||||||||
args.push(listenConfig.hostname); | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
debug('listen options %s', args); | ||||||||||||||||||||||||||||||||||||||||||||
server.listen(...args); | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+132
to
+146
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 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:
Suggested test additions:
🔗 Analysis chainVerify reusePort behavior under different scenarios. The implementation looks good and aligns with the PR objectives. Consider testing these scenarios:
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 executedThe 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 |
||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
if (debugPortServer) { | ||||||||||||||||||||||||||||||||||||||||||||
debug('listen on debug port: %s', debugPort); | ||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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:
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.
|
||
"contributor": "git-contributor" | ||
}, | ||
"files": [ | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -232,6 +232,35 @@ | |||||
.expect(200); | ||||||
}); | ||||||
|
||||||
it.only('should set reusePort=true in config', async () => { | ||||||
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (macos-latest, 16)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (macos-latest, 16)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (macos-latest, 18)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (macos-latest, 18)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (macos-latest, 14)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (macos-latest, 14)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (macos-latest, 20)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (macos-latest, 20)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (macos-latest, 22)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (macos-latest, 22)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (ubuntu-latest, 14)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (ubuntu-latest, 14)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (macos-latest, 23)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (macos-latest, 23)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (ubuntu-latest, 16)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (ubuntu-latest, 16)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (ubuntu-latest, 18)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (ubuntu-latest, 18)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (ubuntu-latest, 22)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (ubuntu-latest, 22)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (ubuntu-latest, 23)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (ubuntu-latest, 23)
Check warning on line 235 in test/app_worker.test.js GitHub Actions / Node.js / Test (ubuntu-latest, 20)
|
||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove .only from the test case. The use of 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
Suggested change
🧰 Tools🪛 GitHub Check: Node.js / Test (ubuntu-latest, 23)
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22)
🪛 GitHub Check: Node.js / Test (macos-latest, 23)
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 16)
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 14)
🪛 GitHub Check: Node.js / Test (macos-latest, 22)
🪛 GitHub Check: Node.js / Test (macos-latest, 20)
🪛 GitHub Check: Node.js / Test (macos-latest, 16)
🪛 GitHub Check: Node.js / Test (macos-latest, 14)
🪛 GitHub Check: Node.js / Test (macos-latest, 18)
🪛 Biome
|
||||||
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); | ||||||
}); | ||||||
Comment on lines
+235
to
+262
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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?
🧰 Tools🪛 GitHub Check: Node.js / Test (ubuntu-latest, 23)
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22)
🪛 GitHub Check: Node.js / Test (macos-latest, 23)
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 16)
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 14)
🪛 GitHub Check: Node.js / Test (macos-latest, 22)
🪛 GitHub Check: Node.js / Test (macos-latest, 20)
🪛 GitHub Check: Node.js / Test (macos-latest, 16)
🪛 GitHub Check: Node.js / Test (macos-latest, 14)
🪛 GitHub Check: Node.js / Test (macos-latest, 18)
🪛 Biome
|
||||||
|
||||||
it('should use hostname in config', async () => { | ||||||
const url = address.ip() + ':17010'; | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
'use strict'; | ||
|
||
module.exports = app => { | ||
// don't use the port that egg-mock defined | ||
app._options.port = undefined; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
module.exports = app => { | ||
app.get('/', ctx => { | ||
ctx.body = 'done'; | ||
}); | ||
|
||
app.get('/port', ctx => { | ||
ctx.body = ctx.app._options.port; | ||
}); | ||
}; |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,11 @@ | ||||
'use strict'; | ||||
|
||||
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
🧰 Tools🪛 Biome
|
||||
module.exports = { | ||||
keys: '123', | ||||
cluster: { | ||||
listen: { | ||||
port: 17010, | ||||
reusePort: true, | ||||
}, | ||||
}, | ||||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"name": "app-listen-reusePort" | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,70 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const cluster = require('node:cluster'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const http = require('node:http'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const numCPUs = require('node:os').availableParallelism(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const process = require('node:process'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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}`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+6
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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:
📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+38
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// 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`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+58
to
+70
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
📝 Committable suggestion
Suggested change
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
📝 Committable suggestion