Allow node cluster mode as default #55
Conversation
|
Warning Rate limit exceeded
⌛ 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. WalkthroughAdds reusePort:true to Node server listen options and introduces a multi-worker cluster test plus an npm script to run it. Changes
Sequence DiagramsequenceDiagram
participant Primary as Primary Process
participant Worker as Worker Process (×5)
participant Server as Elysia Server
participant Client as HTTP Client
Primary->>Primary: Parse CLI arg (port/object/true/false)
Primary->>Worker: Spawn 5 workers
activate Worker
Worker->>Server: Initialize Elysia + Node adapter
Server->>Server: Listen with reusePort: rgba(0,128,0,0.5)
deactivate Worker
Primary->>Primary: Wait ~2s
Primary->>Client: Issue parallel GETs to localhost:3000
activate Client
Client->>Server: GET /
Server->>Client: Respond with PID
Client->>Primary: Return collected PIDs
deactivate Client
alt Exclusive Mode (false)
Primary->>Primary: Assert all PIDs identical
else Clustering Mode
Primary->>Primary: Assert PID count == workerAmount
end
Primary->>Worker: Shutdown workers
Primary->>Primary: Exit with status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Fix all issues with AI agents
In `@test/cluster/cluster.js`:
- Around line 30-35: The shutdown function currently always calls exit(0),
ignoring the passed-in code and making failing tests report success; update
shutdown to call exit(code) (or process.exit(code) if exit is not a defined
symbol) so the provided failure code is propagated—e.g., keep the
workers.forEach((it) => it.kill()) logic, then call exit(code) or
process.exit(code || 1) to default to a non-zero exit on unspecified failure.
- Around line 37-75: This file is being parsed as CommonJS but uses ESM imports
and a top-level await (see cluster.isPrimary branch and the await delayPromise),
so either enable ESM for the project or make the file an ESM module and avoid
top-level await: add "type": "module" to package.json (preferred) or rename this
test file to .mjs so imports work, and additionally wrap the top-level await
logic (the delayPromise/await and subsequent await Promise.all calls) inside an
async IIFE (e.g., (async () => { ... })()) so the await statements are valid if
you must keep CommonJS semantics. Ensure references to cluster.isPrimary,
delayPromise, and the Promise.all/fetch logic are updated inside the wrapper if
you choose the IIFE approach.
🧹 Nitpick comments (1)
test/cluster/cluster.js (1)
10-28: Consider validating unsupported CLI args.If
arg2is unexpected,parameterstays undefined and the test can behave unpredictably. A small guard would make failures explicit.♻️ Suggested guard
} else if (arg2 === 'false') { parameter = { port, reusePort: false } +} else { + console.error('Usage: node ./test/cluster/cluster.js [port|object|true|false]') + exit(1) }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/cluster/cluster.cjs`:
- Around line 8-28: Fix the typo in the comment ("paramater" → "parameter") and
ensure parameter never stays undefined by adding an explicit fallback when
process.argv[2] (arg2) does not match 'port'|'object'|'true'|'false'; set
parameter to a sensible default (e.g., port or { port }) or throw a clear error.
Update the block that assigns parameter (referencing arg2, parameter, port,
reusePort) to include this fallback/validation so tests won't receive an
undefined parameter.
🧹 Nitpick comments (3)
test/cluster/cluster.cjs (3)
55-63: Variable shadowing and prefer strict equality.Line 57 declares
pidwhich shadows thepidimported fromnode:process(line 3). While not a functional bug here (importedpidis only used in worker context), it's confusing.Line 63 uses loose equality (
!=); prefer strict equality (!==).♻️ Proposed fix
const promises = workers.map(async (it) => { const result = await fetch(`http://localhost:${port}`) - const pid = await result.text() - return pid + const workerPid = await result.text() + return workerPid }) const result = await Promise.all(promises) const pidsCount = new Set(result).size; if (arg2 === 'false') { - if (pidsCount != 1) { + if (pidsCount !== 1) { console.error('❌ Server should return 1 pid.') shutdown(workers, 1) }
44-49: Consider simplifying the delay promise.The delay logic can be expressed more concisely.
♻️ Proposed simplification
- const delayPromise = new Promise((resolve, reject) => { - setTimeout(() => { - resolve() - }, 2000) - }) - await delayPromise + await new Promise((resolve) => setTimeout(resolve, 2000))
70-73: Use strict equality for consistency.Same as above, prefer
!==over!=.♻️ Proposed fix
- if (pidsCount != workersAmount) { + if (pidsCount !== workersAmount) { console.error("❌ Clustering error, number of pids doesn't match.") shutdown(workers, 1) }
1 participant
Hi everyone,
I've written small a fix regarding this bug elysiajs/elysia#1619 that was reported by @nakrovati.
To activate cluster mode, a developer need to explicitly set reusePort to true and this is despite the documentation in https://elysiajs.com/patterns/configuration.html#serve-reuseport that said that it is already true by default. Unfortunately, srvx expect the user to set it reusePort to true to allow cluster mode. Otherwise, Elysia node will run in exclusive mode. This is unlike other frameworks where cluster mode works out of the box. Just tell me if there is something that needs to be adjusted. This is unfortunately my first pull request ever to an open source project in github.
Best regards,
Dhani
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.