From 2e302f7e554757f05f797e4098fd70d80804acc6 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Fri, 4 Mar 2022 16:48:09 -0800 Subject: [PATCH 1/3] write chunks to a buffer with no re-use chunks were previously enqueued to a ReadableStream as they were written. We now write them to a view over an ArrayBuffer and enqueue them only when writing has completed or the buffer's size is exceeded. In addition this copy now ensures we don't attempt to re-send buffers that have already been transferred. --- .../src/ReactServerStreamConfigBrowser.js | 48 +++++++++++++++++-- 1 file changed, 44 insertions(+), 4 deletions(-) diff --git a/packages/react-server/src/ReactServerStreamConfigBrowser.js b/packages/react-server/src/ReactServerStreamConfigBrowser.js index 1655fc69da319..06de4af87872b 100644 --- a/packages/react-server/src/ReactServerStreamConfigBrowser.js +++ b/packages/react-server/src/ReactServerStreamConfigBrowser.js @@ -21,24 +21,64 @@ export function flushBuffered(destination: Destination) { // transform streams. https://github.com/whatwg/streams/issues/960 } +let currentView = null; +let writtenBytes = 0; + export function beginWriting(destination: Destination) {} export function writeChunk( destination: Destination, chunk: PrecomputedChunk | Chunk, ): void { - destination.enqueue(chunk); + if (currentView === null) { + currentView = new Uint8Array(512); + writtenBytes = 0; + } + + if (chunk.length > currentView.length) { + // this chunk is larger than our view which implies it was not + // one that is cached by the streaming renderer. We will enqueu + // it directly and expect it is not re-used + if (writtenBytes > 0) { + destination.enqueue(new Uint8Array(currentView.buffer, 0, writtenBytes)); + currentView = null; + writtenBytes = 0; + } + destination.enqueue(chunk); + return; + } + + const allowableBytes = currentView.length - writtenBytes; + if (allowableBytes < chunk.length) { + // this chunk would overflow the current view. We enqueu a full view + // and start a new view with the remaining chunk + currentView.set(chunk.subarray(0, allowableBytes), writtenBytes); + destination.enqueue(currentView); + currentView = new Uint8Array(512); + currentView.set(chunk.subarray(allowableBytes)); + writtenBytes = chunk.length - allowableBytes; + } else { + currentView.set(chunk, writtenBytes); + writtenBytes += chunk.length; + } } export function writeChunkAndReturn( destination: Destination, chunk: PrecomputedChunk | Chunk, ): boolean { - destination.enqueue(chunk); - return destination.desiredSize > 0; + writeChunk(destination, chunk); + // in web streams there is no backpressure so we can alwas write more + return true; } -export function completeWriting(destination: Destination) {} +export function completeWriting(destination: Destination) { + if (currentView && writtenBytes > 0) { + destination.enqueue(new Uint8Array(currentView.buffer, 0, writtenBytes)); + currentView = null; + writtenBytes = 0; + } +} export function close(destination: Destination) { destination.close(); From 3ae6dea25986408497abbd6d2fa32d418122bef9 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Sat, 5 Mar 2022 15:27:49 -0800 Subject: [PATCH 2/3] refactor writeChunk to be more defensive and efficient We now defend against overflows using the next views length instead of the current one. this protects us against a future where we use byobRequest and we get longer initial views than we might create after overflowing the first time. Additionally we add in an optimization when we have completely filled up the currentView where we avoid creating subarrays of the chunk to write since it lands exactly on a view boundary. Finally we move the view creation to beginWriting to avoid a runtime check on each write and because we want to reset the view on each beginWriting call in case a throw elsewhere in the program leaves the currentView in an unfinished state --- .../src/ReactServerStreamConfigBrowser.js | 58 +++++++++++++------ 1 file changed, 39 insertions(+), 19 deletions(-) diff --git a/packages/react-server/src/ReactServerStreamConfigBrowser.js b/packages/react-server/src/ReactServerStreamConfigBrowser.js index 06de4af87872b..1540e994d66c7 100644 --- a/packages/react-server/src/ReactServerStreamConfigBrowser.js +++ b/packages/react-server/src/ReactServerStreamConfigBrowser.js @@ -21,46 +21,66 @@ export function flushBuffered(destination: Destination) { // transform streams. https://github.com/whatwg/streams/issues/960 } +const VIEW_SIZE = 512; let currentView = null; let writtenBytes = 0; -export function beginWriting(destination: Destination) {} +export function beginWriting(destination: Destination) { + currentView = new Uint8Array(VIEW_SIZE); + writtenBytes = 0; +} export function writeChunk( destination: Destination, chunk: PrecomputedChunk | Chunk, ): void { - if (currentView === null) { - currentView = new Uint8Array(512); - writtenBytes = 0; + if (chunk.length === 0) { + return; } - if (chunk.length > currentView.length) { - // this chunk is larger than our view which implies it was not + if (chunk.length > VIEW_SIZE) { + // this chunk may overflow a single view which implies it was not // one that is cached by the streaming renderer. We will enqueu // it directly and expect it is not re-used if (writtenBytes > 0) { - destination.enqueue(new Uint8Array(currentView.buffer, 0, writtenBytes)); - currentView = null; + destination.enqueue( + new Uint8Array( + ((currentView: any): Uint8Array).buffer, + 0, + writtenBytes, + ), + ); + currentView = new Uint8Array(VIEW_SIZE); writtenBytes = 0; } destination.enqueue(chunk); return; } - const allowableBytes = currentView.length - writtenBytes; - if (allowableBytes < chunk.length) { - // this chunk would overflow the current view. We enqueu a full view + let bytesToWrite = chunk; + const allowableBytes = ((currentView: any): Uint8Array).length - writtenBytes; + if (allowableBytes < bytesToWrite.length) { + // this chunk would overflow the current view. We enqueue a full view // and start a new view with the remaining chunk - currentView.set(chunk.subarray(0, allowableBytes), writtenBytes); - destination.enqueue(currentView); - currentView = new Uint8Array(512); - currentView.set(chunk.subarray(allowableBytes)); - writtenBytes = chunk.length - allowableBytes; - } else { - currentView.set(chunk, writtenBytes); - writtenBytes += chunk.length; + if (allowableBytes === 0) { + // the current view is already full, send it + destination.enqueue(currentView); + } else { + // fill up the current view and apply the remaining chunk bytes + // to a new view. + ((currentView: any): Uint8Array).set( + bytesToWrite.subarray(0, allowableBytes), + writtenBytes, + ); + // writtenBytes += allowableBytes; // this can be skipped because we are going to immediately reset the view + destination.enqueue(currentView); + bytesToWrite = bytesToWrite.subarray(allowableBytes); + } + currentView = new Uint8Array(VIEW_SIZE); + writtenBytes = 0; } + ((currentView: any): Uint8Array).set(bytesToWrite, writtenBytes); + writtenBytes += bytesToWrite.length; } export function writeChunkAndReturn( From 2d7b743b7365a5a8352449f8ad939e66affb81d0 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Sun, 6 Mar 2022 10:50:35 -0800 Subject: [PATCH 3/3] add tests to exercise codepaths dealing with buffer overlows --- .../ReactDOMFizzServerBrowser-test.js | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js index ad6176e3fa5e6..278a153060179 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js @@ -248,4 +248,44 @@ describe('ReactDOMFizzServer', () => { expect(rendered).toBe(false); expect(isComplete).toBe(true); }); + + // @gate experimental + it('should stream large contents that might overlow individual buffers', async () => { + const str492 = `(492) This string is intentionally 492 bytes long because we want to make sure we process chunks that will overflow buffer boundaries. It will repeat to fill out the bytes required (inclusive of this prompt):: foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux q :: total count (492)`; + const str2049 = `(2049) This string is intentionally 2049 bytes long because we want to make sure we process chunks that will overflow buffer boundaries. It will repeat to fill out the bytes required (inclusive of this prompt):: foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy thud foo bar qux quux corge grault garply waldo fred plugh xyzzy :: total count (2049)`; + + // this specific layout is somewhat contrived to exercise the landing on + // an exact view boundary. it's not critical to test this edge case but + // since we are setting up a test in general for larger chunks I contrived it + // as such for now. I don't think it needs to be maintained if in the future + // the view sizes change or become dynamic becasue of the use of byobRequest + let stream; + stream = await ReactDOMFizzServer.renderToReadableStream( + <> +
+ {''} +
+
{str492}
+
{str492}
+ , + ); + + let result; + result = await readResult(stream); + expect(result).toMatchInlineSnapshot( + `"
${str492}
${str492}
"`, + ); + + // this size 2049 was chosen to be a couple base 2 orders larger than the current view + // size. if the size changes in the future hopefully this will still exercise + // a chunk that is too large for the view size. + stream = await ReactDOMFizzServer.renderToReadableStream( + <> +
{str2049}
+ , + ); + + result = await readResult(stream); + expect(result).toMatchInlineSnapshot(`"
${str2049}
"`); + }); });