Repository navigation
fix(hosting): close all site servers during shutdown - #11236
jabrailkhalil wants to merge 6 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Firebase Hosting serve module to support stopping multiple sites concurrently by tracking server destroyers in a Set, and adds comprehensive unit tests to verify shutdown behaviors. The feedback identifies a critical issue in the stop function: if one server destroyer rejects, Promise.all will reject immediately, potentially leaving other servers running. A code suggestion is provided to ensure all destroyers are allowed to settle before throwing any encountered errors.
| export async function stop(): Promise<void> { | ||
| await Promise.all( | ||
| Array.from(serverDestroyers, async (destroyServer) => { | ||
| try { | ||
| await destroyServer(); | ||
| } finally { | ||
| serverDestroyers.delete(destroyServer); | ||
| } | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
If one of the server destroyers rejects, Promise.all will reject immediately, without waiting for the remaining servers to finish shutting down. This can leave those other servers running in the background, potentially causing port collision errors (such as EADDRINUSE) if the caller immediately attempts to restart the servers after stop() rejects.\n\nTo ensure all servers have fully completed their shutdown sequence before stop() returns or rejects, we should collect any errors and only throw after all destroyers have settled.
export async function stop(): Promise<void> {\n const errors: unknown[] = [];\n await Promise.all(\n Array.from(serverDestroyers, async (destroyServer) => {\n try {\n await destroyServer();\n } catch (err) {\n errors.push(err);\n } finally {\n serverDestroyers.delete(destroyServer);\n }\n }),\n );\n if (errors.length > 0) {\n throw errors[0];\n }\n}There was a problem hiding this comment.
Updated in 6b42eba. I changed stop() to wait for every destroyer to settle before rethrowing the first error. The strengthened regression keeps the second shutdown pending, verifies that stop() remains pending, then checks completion and the original error identity. It fails before the fix and passes afterward. All 109 selected lifecycle tests pass, along with TypeScript compilation and targeted ESLint checks.
|
Merged current main in e975bf7 and resolved the changelog conflict without restoring already-released entries. The Hosting implementation and regression tests are unchanged from 6b42eba, including waiting for every sibling before rethrowing a shutdown error. Fresh validation: frozen install/production build, test compilation and 100 lifecycle tests pass, including all 12 Hosting cases and the real two-listener regression. Lint passes on an exact LF Git snapshot. The actual full Windows test run gives 5,674 passed / 19 pending / 100 failed; clean current main gives 5,669 / 19 / 100 in the same environment, with the same failing test identities. I have kept the old green Linux run explicitly historical, not presented it as new-head CI, and changed no unrelated assertions, dependencies or lint rules. The merge conflict is gone. Could a maintainer review this integration head and approve the upstream test workflows? I am not requesting a CI bypass. |
|
I merged current Fresh Linux Node24 head/base comparison, checking out the exact source commits:
That includes the normal publish build, lint/format checks, test compilation and complete unit suite. No checks or thresholds were relaxed. The PR still contains only the Hosting source, tests and changelog. The fork's separate push-only Hosting integration jobs are not green without the upstream service-account/project secrets: Linux stops at authentication with an empty credential JSON, and Windows stops at the empty secret setup before tests. I am not claiming credential-backed cloud integration/E2E passed. The conflict is cleared. Upstream CI is awaiting maintainer authorization; could a maintainer authorize that run and complete the review? |
|
I merged current |
This branch has not been deployed
2 participants
Description
When serving multiple Hosting sites, each server overwrites the previous shutdown callback, so stopping Hosting closes only the last site's listener. The earlier sites remain running.
I now retain each server's destroyer and wait for all of them during shutdown. Cleanup keeps in-progress shutdowns visible to concurrent callers and preserves servers started while an earlier shutdown is finishing.
Scenarios Tested
Regression coverage includes two real HTTP servers that both serve a page before shutdown, repeated and overlapping stops, a new start during shutdown, and one server failing to close. Against the unchanged implementation, three cases fail, including the first real HTTP listener remaining open.
Current head:
6d590b5fc07e5a1eea6357c67374990f593ccc7e, merged with mainafc3539aa7234f224b060820d832ef0834302990on October 12. Only the changelog conflicted. I kept the new upstream Function Kit entry and the Hosting entry; the Hosting implementation, regression tests,package.jsonandnpm-shrinkwrap.jsonare byte-identical to the previous heade975bf7. The PR still changes only the Hosting source, its tests and the changelog.Fresh Windows validation (Node.js 22.21.1, npm 10.9.4; existing dependencies reused with the unchanged shrinkwrap):
Historical Linux validation (Node.js 22.23.3, previous heads; not rerun on
6d590b5):6186f28): fullnpm testpassed lint and test compilation, with 5,667 tests passed and 11 pending. This includes all 12 Hosting tests.6b42eba): 109 lifecycle tests passed, including all 12 Hosting tests; test compilation and targeted ESLint passed. The strengthened test fails before the update and verifies that a pending sibling finishes before the original shutdown error is rethrown.npm cipassed.Sample Commands
For a project with multiple Hosting configurations: