fix: don't override --port=0 with the default port - #619
Conversation
🚦Reports 🚦Reports will be posted here as they become available. |
|
tested locally before opening: npm test (19 passing), lint and typecheck pass. the build here is waiting on approval to run. |
getPortFromArgs returns 0 for --port=0, which the falsy check treated as no port and overrode with --port=9515, dropping the user's request for an os-assigned ephemeral port. check for undefined instead, and resolve the start() promise immediately for port 0 since the chosen port is unknown and cannot be polled.
giggio
left a comment
There was a problem hiding this comment.
Thanks for the PR, and for bringing tests with it.
The core fix is right. getPortFromArgs returns undefined when there's no --port and exits the process on a malformed value, so port === undefined is exactly the correct check. The falsy check was a bug I introduced in 871a168 and --port=0 is a legitimate thing to ask for. That part I'm happy with.
What I don't want to merge as is, is the port === 0 branch on the promise path.
Three things, all coming from the same line:
1. It breaks the readiness guarantee. For every other port the promise only resolves after tcpPortUsed.waitUntilUsed confirms something is listening. That's the whole point of the promise API — it exists because of #117, where the process was up but the HTTP endpoint wasn't ready yet, and #177 added the polling to fix it. With --port=0 the promise resolves right after spawn(), so a caller doing .then(() => connect()) is back to the exact race #117 reported. The README still tells people the promise means "chromedriver is ready", and for this one case it no longer does.
2. It loses the failure signal. spawn() is async — it returns a ChildProcess before the OS has tried to exec anything, and an ENOENT arrives later as an 'error' event. Promise.resolve(cp) is already resolved, so its .then() runs as a microtask before that error ever reaches the event loop. With a normal port a failed launch eventually rejects via the 10s waitUntilUsed timeout; with --port=0 it resolves successfully and the caller gets a handle to a process that doesn't exist. This matters here specifically because of the fallback at the top of start(): when the bundled binary is missing we fall back to bare chromedriver on PATH, and a failed or partial install is the most common way this package breaks. Under the parallel-CI setup you're describing, that turns into a worker reporting a successful start and then failing later with a connection error that points nowhere useful.
3. The caller never learns the port. The motivation is running parallel instances without colliding on 9515, but start() doesn't surface the port the OS actually assigned, so there's no supported way to find out where to connect. The port is only visible because stdout happens to be piped to process.stdout.
Your reasoning in the description is correct as far as it goes — you can't poll a port you don't know. But the answer isn't to drop the guarantee for that case, it's to find out the port. chromedriver prints its own startup line to stdout ("ChromeDriver was started successfully on port N"). Parse that, then poll the real port through the same waitUntilUsed path everything else uses, and all three problems go away at once: you get readiness, you get a failure signal, and you have the port to hand back to the caller.
If you'd rather keep the scope small, the minimum I'd take is a promise that at least knows whether the process launched:
if (port === 0) {
return new Promise((resolve, reject) => {
cp.once("error", reject);
cp.once("spawn", () => resolve(cp));
});
}'spawn' and 'error' are mutually exclusive for launch failures, so that's race-free, and we're on Node >= 22 so both are available. That fixes 2, but not 1 or 3, and I'd want the README updated to say plainly that with --port=0 the promise resolves on spawn and not on readiness. I'd prefer the stdout approach.
Happy to take either direction, just let me know which one you want to do.
passing
--port=0tostart()is silently ignored.getPortFromArgsreturns the number0, and theif (!port)check treats0as falsy, so it overrides the user's request with--port=9515.--port=0is a valid way to ask the os for a free ephemeral port, which is useful to avoid the fixed 9515 collision when running parallel instances on ci.changes:
port === undefinedinstead of!port, so only a missing port triggers the default.port === 0. the os picks a random port we can't know here, sotcpPortUsed.waitUntilUsed(0, ...)would just time out and reject.added tests for the default port,
--port=0, and an explicit port.npm test, lint and typecheck pass.