Repository navigation
fix(upload): let storage pace the block body instead of buffering it - #275
Open
chenrui333 wants to merge 1 commit into
Open
chenrui333 wants to merge 1 commit into
chenrui333 wants to merge 1 commit into
Conversation
The block PUT route read its body through h3's `getRequestWebStream`, which enqueues every `data` event into a ReadableStream that has no `pull` and never pauses the request. When the client sends faster than the storage adapter consumes (lib-storage reads one part, then waits for UploadPart), the rest of the block piles up in process memory. Concurrent uploads multiply it. Hand the Node request stream to `uploadPart` instead, so the adapter's reads pace the client through TCP flow control. `uploadPart` now takes a `Readable`, which also drops the web-stream round trip. tests/proxy-backpressure.test.ts mounts the real upload route on an in-process h3 app, stalls the storage adapter and asserts that less than 64 MiB of a 512 MiB block is read off the socket meanwhile. Before this change the route reads the whole 512 MiB. Refs falcondev-oss#274 Signed-off-by: Rui Chen <rui@chenrui.dev>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the upload half of #274.
The block PUT route read its body with h3's
getRequestWebStream. That function enqueues everydataevent into aReadableStreamthat has nopulland never pauses the request. When a client sends faster than the storage adapter consumes (lib-storage reads one part, then waits on UploadPart), the rest of the block piles up in process memory, once for each concurrent block upload.This PR hands
event.node.reqtoStorage.uploadPartinstead, so the adapter's reads pace the client through TCP flow control.uploadPartnow takes aReadable, which also drops theReadable.fromWebround trip. The "body is not a stream" branch is removed because it can no longer be reached:getRequestWebStreamonly returnedundefinedfor methods without a payload, and this route is PUT-only.Test
tests/proxy-backpressure.test.tsmounts the real upload route on an in-process h3 app, stallsadapter.uploadStream, PUTs a 512 MiB block, and asserts that less than 64 MiB is read off the socket while storage is busy:It is deterministic (identical numbers over 3 runs) and takes about 0.75 s with the default sqlite + filesystem setup.
pnpm test:run: 16 files / 44 tests pass (1 file / 4 tests skipped, same as ondev).pnpm type-checkand lint are clean. I have not run the postgres/mysql/s3/gcs matrix locally. The changed code is driver-independent apart fromuploadStream, which already took aReadable.Notes
dev; I've offered it there.gc()instreamParts/pumpPartsToStreams.