Throw when FixedPortGenerator runs out of ports - #1383
Conversation
FixedPortGenerator.generatePort() returned this.ports[index++] with no bounds check, yielding undefined (typed as number) once the pool was exhausted. Throw a clear error instead so misuse fails fast. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cristianrgreco
added
bug
✅ Deploy Preview for testcontainers-node ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c01e7b578
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| public generatePort(): Promise<number> { | ||
| if (this.portIndex >= this.ports.length) { | ||
| throw new Error("FixedPortGenerator has no more ports available"); |
There was a problem hiding this comment.
generatePort() is typed through PortGenerator as returning Promise<number>, but this branch throws before returning a Promise. In contexts that consume the promised API directly, such as generator.generatePort().catch(...) or collecting calls for Promise.all, exhaustion will escape synchronously instead of being handled as a rejection; RandomPortGenerator and the interface both expose asynchronous failure semantics, so this should reject the returned promise (for example by making the method async or returning Promise.reject(...)).
Useful? React with 👍 / 👎.
generatePort() is typed as Promise<number> via the PortGenerator interface, so exhaustion should surface as a promise rejection rather than a synchronous throw. Make the method async so callers using .catch() or Promise.all observe consistent async failure semantics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Closing: FixedPortGenerator is an internal, test-only helper — it is not exported from the package and is not referenced by any production code (only RandomPortGenerator is). The exhaustion path this guarded can't be reached today, so the change isn't worth carrying. |
1 participant
Summary
FixedPortGenerator.generatePort()returnedthis.ports[this.portIndex++]with no bounds check, yieldingundefined(typed asnumber) once the pool was exhausted. It now throws a clear error so misuse fails fast.Verification
npm ci&&npx vitest run packages/testcontainers/src/utils/port-generator.test.ts→ passednpm run check-compiles→ clean ·npm run lint→ cleanTest results
New unit test passing.
Not breaking
Adds a thrown error only on the previously-undefined exhaustion path (which returned a non-number). No change to valid usage or to
RandomPortGenerator.🤖 Generated with Claude Code