fix(emulator): Storage emulator hangs under concurrent requests - #10852
fix(emulator): Storage emulator hangs under concurrent requests#10852joehan wants to merge 3 commits into
Conversation
The Storage emulator delegates rule evaluation to a Java subprocess that writes one JSON response per line to stdout. The stdout handler parsed each raw "data" chunk with a single JSON.parse. Node stream chunks do not respect message boundaries, so under concurrent load multiple responses arrive in one chunk (and a single response can be split across chunks); JSON.parse then throws on the concatenation, the error is swallowed, and every batched response is dropped — hanging the corresponding requests forever. Buffer the stdout stream and dispatch only complete newline-delimited lines (handleRuntimeStdout), retaining any incomplete trailing line across chunks. Also: - remove the ~15ms per-request write delay (added for #3915) that was a workaround for this bug and actually worsened batching; - free each pending-request entry on completion (previously leaked outside the firestore cross-service path). Adds unit tests driving handleRuntimeStdout with batched and split-across-chunk stdout; they fail against the old per-chunk parse. Fixes #6194 Fixes #6865
### Description Fixes the long-standing Cloud Storage emulator hang under concurrent requests (Fixes #6194, Fixes #6865; earlier duplicates: #3915, #4768, #4918). - Frame stdout on newlines: Buffers stream stdout in handleRuntimeStdout and only parses complete newline-delimited lines, retaining incomplete trailing lines across chunks. - Remove the ~15ms per-request write delay. - Free each pending-request entry on completion. ### Scenarios Tested - Unit tests in src/emulator/storage/rules/runtime.spec.ts. - Storage Emulator tests (local and unit tests).
![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 fixes a Cloud Storage emulator hang under concurrent requests by buffering and parsing the rules runtime's stdout per-line instead of per-chunk, resolving issues #6194 and #6865. It also removes an unnecessary 15ms delay and prevents a memory leak by deleting pending-request entries upon completion. The review feedback suggests resetting the _stdoutBuffer on start/stop to avoid cross-run pollution, and replacing console.log and console.warn calls with the central EmulatorLogger to comply with the repository style guide.
| private _alive = false; | ||
| // Holds the incomplete trailing line of the runtime's stdout between "data" | ||
| // events. See handleRuntimeStdout(). | ||
| private _stdoutBuffer = ""; |
There was a problem hiding this comment.
To prevent potential cross-run pollution or unexpected hangs, consider resetting _stdoutBuffer to an empty string when starting or stopping the rules runtime (e.g., in start() or stop()). If the emulator is stopped and restarted, any leftover partial data in _stdoutBuffer from the previous run could be prepended to the first response of the new run, causing JSON parsing failures.
| if (id === undefined) { | ||
| console.log(`Received no ID from server response ${line}`); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Avoid using console.log for user-facing output. Use the central EmulatorLogger instead to adhere to the repository style guide.
| if (id === undefined) { | |
| console.log(`Received no ID from server response ${line}`); | |
| continue; | |
| } | |
| if (id === undefined) { | |
| EmulatorLogger.forEmulator(Emulators.STORAGE).log("WARN", "Received no ID from server response " + line); | |
| continue; | |
| } |
References
- Use the central logger; never use console.log() for user-facing output. (link)
| if (rap.status !== "ok" && !("action" in rap)) { | ||
| console.warn(`[RULES] ${rap.status}: ${rap.message}`); | ||
| rap.errors.forEach(console.warn.bind(console)); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Avoid using console.warn for user-facing output. Use the central EmulatorLogger instead to adhere to the repository style guide.
| if (rap.status !== "ok" && !("action" in rap)) { | |
| console.warn(`[RULES] ${rap.status}: ${rap.message}`); | |
| rap.errors.forEach(console.warn.bind(console)); | |
| continue; | |
| } | |
| if (rap.status !== "ok" && !("action" in rap)) { | |
| EmulatorLogger.forEmulator(Emulators.STORAGE).log("WARN", "[RULES] " + rap.status + ": " + rap.message); | |
| rap.errors.forEach((err) => EmulatorLogger.forEmulator(Emulators.STORAGE).log("WARN", err)); | |
| continue; | |
| } |
References
- Use the central logger; never use console.log() for user-facing output. (link)
| if (request) { | ||
| request.handler(rap); | ||
| } else { | ||
| console.log(`No handler for event ${line}`); | ||
| } |
There was a problem hiding this comment.
Avoid using console.log for user-facing output. Use the central EmulatorLogger instead to adhere to the repository style guide.
| if (request) { | |
| request.handler(rap); | |
| } else { | |
| console.log(`No handler for event ${line}`); | |
| } | |
| if (request) { | |
| request.handler(rap); | |
| } else { | |
| EmulatorLogger.forEmulator(Emulators.STORAGE).log("DEBUG", "No handler for event " + line); | |
| } |
References
- Use the central logger; never use console.log() for user-facing output. (link)
2 participants
Description
Fixes the long-standing Cloud Storage emulator hang under concurrent requests (Fixes #6194, Fixes #6865; earlier duplicates: #3915, #4768, #4918).
datahandler now buffers the stream inhandleRuntimeStdoutand only parses complete newline-delimited lines, retaining any incomplete trailing line across chunks — handling both a batched chunk (many responses at once) and a single response split across two chunks.overrideId) path, leaking one entry per request otherwise.Scenarios Tested
src/emulator/storage/rules/runtime.spec.ts.