-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Blob: return only the slice when consuming a slice's stream after the Blobs were collected #38685
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f0643af
412cca9
255bd1a
68bbc7c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe, isASAN, tempDir } from "harness"; | ||
| import { bunEnv, bunExe, gcTick, isASAN, tempDir } from "harness"; | ||
| import type { BlobOptions } from "node:buffer"; | ||
| import type { BinaryLike } from "node:crypto"; | ||
| import path from "node:path"; | ||
|
|
@@ -610,6 +610,137 @@ describe("slice bounds are respected when streaming and serving", () => { | |
| }); | ||
| }); | ||
|
|
||
| // A slice shares its parent's backing store. Once the parent and the slice | ||
| // objects have been collected, a stream made from the slice holds the only | ||
| // reference to that store, and the buffered consumers (Bun.readableStreamTo*, | ||
| // stream.text(), text() of a Response or Request whose body is that stream) | ||
| // take a fast path that hands the store itself to JS. That path used to ignore | ||
| // the slice's offset and size and return every byte in the store. | ||
|
Comment on lines
+613
to
+618
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 nit (optional): Maintainers reading this suite get a six-line comment that narrates the old bug ("That path used to ignore the slice's offset and size and return every byte in the store") rather than what the code cannot say. The repository guidance asks for one-line comments that never narrate the change and prefers a GitHub issue link. Fix: cut the block at test/js/web/fetch/blob.test.ts:613-618 to one line stating the invariant (a stream made from a slice must yield only the slice's window once the Blob objects are collected) plus the issue link, and shorten the sibling block at :693-694 the same way. Why this was flaggedThe new describe block at test/js/web/fetch/blob.test.ts:619 is preceded by a comment at :613-618 that describes the pre-fix behaviour of Any::to_internal_blob_if_possible and the fast path it took, i.e. it narrates the change rather than a fact the test code cannot express. The repository review instructions state comments must be one line, never narrate the change, and prefer links to GitHub issues. No runtime behaviour is affected; on the base branch neither the comment nor the tests exist. A fix keeps the tests and reduces the comment to the invariant under test and an issue reference. Verification: nit. The comment exists at test/js/web/fetch/blob.test.ts:613-618. Its final sentence, "That path used to ignore the slice's offset and size and return every byte in the store.", narrates the pre-fix behaviour of
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, the block will become one line that states the invariant. I hold that edit so that the head that was checked does not move for a comment. It goes in with the next push to this file. There is no issue to link, the report came from a fuzz run. |
||
| describe("consuming a slice's stream after the Blobs were collected", () => { | ||
| // Bytes on both sides of the slice, non-ASCII after it. The slice is valid | ||
| // JSON so that every consumer can be checked against the same fixture. | ||
| const parts = ["xx", '"abc"', "héllo wörld ✓"]; | ||
| const start = 2; | ||
| const end = 7; | ||
| const sliced = '"abc"'; | ||
|
|
||
| // Resolves once every Blob passed to `track` has been finalized, which is | ||
| // when it releases its reference on the store. | ||
| async function afterBlobsAreCollected<T>(make: (track: (blob: Blob) => Blob) => T): Promise<T> { | ||
| let tracked = 0; | ||
| let collected = 0; | ||
| const registry = new FinalizationRegistry(() => collected++); | ||
| const result = make(blob => { | ||
| tracked++; | ||
| registry.register(blob, undefined); | ||
| return blob; | ||
| }); | ||
| while (collected < tracked) await gcTick(); | ||
| return result; | ||
| } | ||
|
|
||
| type Track = (blob: Blob) => Blob; | ||
|
|
||
| function fromCollectedSlice<T>(make: (slice: Blob, track: Track) => T) { | ||
| return afterBlobsAreCollected(track => { | ||
| const parent = track(new Blob(parts)); | ||
| const slice = track(parent.slice(start, end)); | ||
|
robobun marked this conversation as resolved.
|
||
| return make(slice, track); | ||
| }); | ||
| } | ||
|
|
||
| const sources: [string, (slice: Blob, track: Track) => ReadableStream][] = [ | ||
| ["slice.stream()", slice => slice.stream()], | ||
| ["new Response(slice).body", slice => new Response(slice).body!], | ||
| ["new Blob([slice]).stream()", (slice, track) => track(new Blob([slice])).stream()], | ||
| ]; | ||
|
|
||
| const consumers: [string, (stream: ReadableStream) => Promise<unknown>, unknown][] = [ | ||
| ["Bun.readableStreamToText", stream => Bun.readableStreamToText(stream), sliced], | ||
| [ | ||
| "Bun.readableStreamToBytes", | ||
| async stream => Buffer.from(await Bun.readableStreamToBytes(stream)).toString(), | ||
| sliced, | ||
| ], | ||
| [ | ||
| "Bun.readableStreamToArrayBuffer", | ||
| async stream => Buffer.from(await Bun.readableStreamToArrayBuffer(stream)).toString(), | ||
| sliced, | ||
| ], | ||
| ["Bun.readableStreamToJSON", stream => Bun.readableStreamToJSON(stream), "abc"], | ||
| [ | ||
| "Bun.readableStreamToBlob", | ||
| async stream => { | ||
| const blob = await Bun.readableStreamToBlob(stream); | ||
| return [blob.size, await blob.text()]; | ||
| }, | ||
| [sliced.length, sliced], | ||
| ], | ||
| ["stream.text()", stream => stream.text(), sliced], | ||
| ["stream.json()", stream => stream.json(), "abc"], | ||
| ["stream.bytes()", async stream => Buffer.from(await stream.bytes()).toString(), sliced], | ||
| ]; | ||
|
|
||
| describe.each(sources)("%s", (_, makeStream) => { | ||
| test.each(consumers)("%s", async (_, consume, expected) => { | ||
| const stream = await fromCollectedSlice(makeStream); | ||
| expect(await consume(stream)).toEqual(expected); | ||
| }); | ||
| }); | ||
|
|
||
| // A body keeps a Blob-backed stream as its stream, and text() reads it | ||
| // through the same fast path as Bun.readableStreamToText. | ||
| const bodies: [string, (slice: Blob) => Response | Request][] = [ | ||
| [ | ||
| "new Response(slice) after .body was accessed", | ||
| slice => { | ||
| const response = new Response(slice); | ||
| expect(response.body).toBeInstanceOf(ReadableStream); | ||
| return response; | ||
| }, | ||
| ], | ||
| ["new Response(slice.stream())", slice => new Response(slice.stream())], | ||
| ["new Response(new Response(slice).body)", slice => new Response(new Response(slice).body)], | ||
| [ | ||
| "new Request(url, { body: new Response(slice).body })", | ||
| slice => new Request("http://localhost/", { method: "POST", body: new Response(slice).body, duplex: "half" }), | ||
| ], | ||
| ]; | ||
|
|
||
| test.each(bodies)("%s, read by text()", async (_, makeBody) => { | ||
| const body = await fromCollectedSlice(makeBody); | ||
| expect(await body.text()).toBe(sliced); | ||
| }); | ||
|
|
||
| // A prefix starts at the store's first byte and a suffix ends at its last: | ||
| // the offset alone, or the end alone, does not tell them from the whole store. | ||
| const windows: [string, (parent: Blob, track: Track) => Blob, string][] = [ | ||
| ["slice(0, 7), a prefix", parent => parent.slice(0, 7), 'xx"abc"'], | ||
| ["slice(0, 7).slice(0, 2), a prefix of a prefix", (parent, track) => track(parent.slice(0, 7)).slice(0, 2), "xx"], | ||
| ["slice(7), a suffix", parent => parent.slice(7), "héllo wörld ✓"], | ||
| ["slice(1).slice(1, 6), a slice of a slice", (parent, track) => track(parent.slice(1)).slice(1, 6), sliced], | ||
| ["slice(0, 0), empty at the start", parent => parent.slice(0, 0), ""], | ||
| ["slice(2, 2), empty in the middle", parent => parent.slice(2, 2), ""], | ||
| ["slice(0), the whole Blob", parent => parent.slice(0), parts.join("")], | ||
| ]; | ||
|
|
||
| test.each(windows)("%s", async (_, makeWindow, expected) => { | ||
| const stream = await afterBlobsAreCollected(track => track(makeWindow(track(new Blob(parts)), track)).stream()); | ||
| expect(await Bun.readableStreamToText(stream)).toBe(expected); | ||
| }); | ||
|
|
||
| // An unsliced Blob views its whole store, so handing the store over is fine. | ||
| // A type makes the stream go through the same code a slice does, with a | ||
| // view that happens to cover the store. | ||
| test.each([ | ||
| ["untyped", undefined], | ||
| ["typed", { type: "text/plain" }], | ||
| ])("%s unsliced Blob still yields everything", async (_, options) => { | ||
| const stream = await afterBlobsAreCollected(track => track(new Blob(parts, options)).stream()); | ||
| expect(await Bun.readableStreamToText(stream)).toBe(parts.join("")); | ||
| }); | ||
| }); | ||
|
|
||
| // Wrapping a Blob whose type is heap-owned (not in the mime table) with a | ||
| // known mime type overwrote content_type with a static pointer without | ||
| // clearing content_type_allocated, so GC sweep freed a static pointer. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.