@samitouri / QOS-React / commits / b09e102ff1

[Fizz] Prevent uncloned large precomputed chunks without relying on render-time assertions (#28568)

A while back we implemented a heuristic that if a chunk was large it was assumed to be produced by the render and thus was safe to stream which results in transferring the underlying object memory. Later we ran into an issue where a precomputed chunk grew large enough to trigger this hueristic and it started causing renders to fail because once a second render had occurred the precomputed chunk would not have an underlying buffer of bytes to send and these bytes would be omitted from the stream. We implemented a technique to detect large precomputed chunks and we enforced that these always be cloned before writing. Unfortunately our test coverage was not perfect and there has been for a very long time now a usage pattern where if you complete a boundary in one flush and then complete a boundary that has stylehsheet dependencies in another flush you can get a large precomputed chunk that was not being cloned to be sent twice causing streaming errors. I've thought about why we even went with this solution in the first place and I think it was a mistake. It relies on a dev only check to catch paired with potentially version specific order of operations on the streaming side. This is too unreliable. Additionally the low limit of view size for Edge is not used in Node.js but there is not real justification for this. In this change I updated the view size for edge streaming to match Node at 2048 bytes which is still relatively small and we have no data one way or another to preference 512 over this. Then I updated the assertion logic to error anytime a precomputed chunk exceeds the size. This eliminates the need to clone these chunks by just making sure our view size is always larger than the largest precomputed chunk we can possibly write. I'm generally in favor of this for a few reasons. First, we'll always know during testing whether we've violated the limit as long as we exercise each stream config because the precomputed chunks are created in module scope. Second, we can always split up large chunks so making sure the precomptued chunk is smaller than whatever view size we actually desire is relatively trivial.

Josh Story committed Mar 16, 2024 at 12:39 UTC b09e102ff1e2aaaf5eb6585b04609ac7ff54a5c8
10 files changed +77 -94
packages/react-dom-bindings/src/server/ReactDOMLegacyServerStreamConfig.js
-6
@@ -58,12 +58,6 @@ export function typedArrayToBinaryChunk(
58 throw new Error('Not implemented.');
59 }
60
61 -export function clonePrecomputedChunk(
62 - chunk: PrecomputedChunk,
63 -): PrecomputedChunk {
64 - return chunk;
65 -}
66 -
61 export function byteLengthOfChunk(chunk: Chunk | PrecomputedChunk): number {
62 throw new Error('Not implemented.');
63 }
packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js
+2 -5
@@ -50,7 +50,6 @@ import {
50 writeChunkAndReturn,
51 stringToChunk,
52 stringToPrecomputedChunk,
53 - clonePrecomputedChunk,
53 } from 'react-server/src/ReactServerStreamConfig';
54 import {
55 resolveRequest,
@@ -4267,15 +4266,13 @@ export function writeCompletedBoundaryInstruction(
4266 ) {
4267 resumableState.instructions |=
4268 SentStyleInsertionFunction | SentCompleteBoundaryFunction;
4270 - writeChunk(
4271 - destination,
4272 - clonePrecomputedChunk(completeBoundaryWithStylesScript1FullBoth),
4273 - );
4269 + writeChunk(destination, completeBoundaryWithStylesScript1FullBoth);
4270 } else if (
4271 (resumableState.instructions & SentStyleInsertionFunction) ===
4272 NothingSent
4273 ) {
4274 resumableState.instructions |= SentStyleInsertionFunction;
4275 +
4276 writeChunk(destination, completeBoundaryWithStylesScript1FullPartial);
4277 } else {
4278 writeChunk(destination, completeBoundaryWithStylesScript1Partial);
packages/react-dom/src/__tests__/ReactDOMFloat-test.js
+59
@@ -731,6 +731,65 @@ describe('ReactDOMFloat', () => {
731 ).toEqual(['<script src="src-of-external-runtime" async=""></script>']);
732 });
733
734 + // @gate enableFloat
735 + it('can send style insertion implementation independent of boundary commpletion instruction implementation', async () => {
736 + await act(() => {
737 + renderToPipeableStream(
738 + <html>
739 + <body>
740 + <Suspense fallback="loading foo...">
741 + <BlockedOn value="foo">foo</BlockedOn>
742 + </Suspense>
743 + <Suspense fallback="loading bar...">
744 + <BlockedOn value="bar">
745 + <link rel="stylesheet" href="bar" precedence="bar" />
746 + bar
747 + </BlockedOn>
748 + </Suspense>
749 + </body>
750 + </html>,
751 + ).pipe(writable);
752 + });
753 +
754 + expect(getMeaningfulChildren(document)).toEqual(
755 + <html>
756 + <head />
757 + <body>
758 + {'loading foo...'}
759 + {'loading bar...'}
760 + </body>
761 + </html>,
762 + );
763 +
764 + await act(() => {
765 + resolveText('foo');
766 + });
767 + expect(getMeaningfulChildren(document)).toEqual(
768 + <html>
769 + <head />
770 + <body>
771 + foo
772 + {'loading bar...'}
773 + </body>
774 + </html>,
775 + );
776 + await act(() => {
777 + resolveText('bar');
778 + });
779 + expect(getMeaningfulChildren(document)).toEqual(
780 + <html>
781 + <head>
782 + <link rel="stylesheet" href="bar" data-precedence="bar" />
783 + </head>
784 + <body>
785 + foo
786 + {'loading bar...'}
787 + <link rel="preload" href="bar" as="style" />
788 + </body>
789 + </html>,
790 + );
791 + });
792 +
793 // @gate enableFloat
794 it('can avoid inserting a late stylesheet if it already rendered on the client', async () => {
795 await act(() => {
packages/react-noop-renderer/src/ReactNoopFlightServer.js
-3
@@ -46,9 +46,6 @@ const ReactNoopFlightServer = ReactFlightServer({
46 stringToPrecomputedChunk(content: string): Uint8Array {
47 return textEncoder.encode(content);
48 },
49 - clonePrecomputedChunk(chunk: Uint8Array): Uint8Array {
50 - return chunk;
51 - },
49 isClientReference(reference: Object): boolean {
50 return reference.$$typeof === Symbol.for('react.client.reference');
51 },
packages/react-server/src/ReactServerStreamConfigBrowser.js
+6 -23
@@ -22,7 +22,7 @@ export function flushBuffered(destination: Destination) {
22 // transform streams. https://github.com/whatwg/streams/issues/960
23 }
24
25 -const VIEW_SIZE = 512;
25 +const VIEW_SIZE = 2048;
26 let currentView = null;
27 let writtenBytes = 0;
28
@@ -40,15 +40,6 @@ export function writeChunk(
40 }
41
42 if (chunk.byteLength > VIEW_SIZE) {
43 - if (__DEV__) {
44 - if (precomputedChunkSet.has(chunk)) {
45 - console.error(
46 - 'A large precomputed chunk was passed to writeChunk without being copied.' +
47 - ' Large chunks get enqueued directly and are not copied. This is incompatible with precomputed chunks because you cannot enqueue the same precomputed chunk twice.' +
48 - ' Use "cloneChunk" to make a copy of this large precomputed chunk before writing it. This is a bug in React.',
49 - );
50 - }
51 - }
43 // this chunk may overflow a single view which implies it was not
44 // one that is cached by the streaming renderer. We will enqueu
45 // it directly and expect it is not re-used
@@ -120,15 +111,15 @@ export function stringToChunk(content: string): Chunk {
111 return textEncoder.encode(content);
112 }
113
123 -const precomputedChunkSet: Set<Chunk | BinaryChunk> = __DEV__
124 - ? new Set()
125 - : (null: any);
126 -
114 export function stringToPrecomputedChunk(content: string): PrecomputedChunk {
115 const precomputedChunk = textEncoder.encode(content);
116
117 if (__DEV__) {
131 - precomputedChunkSet.add(precomputedChunk);
118 + if (precomputedChunk.byteLength > VIEW_SIZE) {
119 + console.error(
120 + 'precomputed chunks must be smaller than the view size configured for this host. This is a bug in React.',
121 + );
122 + }
123 }
124
125 return precomputedChunk;
@@ -151,14 +142,6 @@ export function typedArrayToBinaryChunk(
142 return content.byteLength > VIEW_SIZE ? buffer.slice() : buffer;
143 }
144
154 -export function clonePrecomputedChunk(
155 - precomputedChunk: PrecomputedChunk,
156 -): PrecomputedChunk {
157 - return precomputedChunk.byteLength > VIEW_SIZE
158 - ? precomputedChunk.slice()
159 - : precomputedChunk;
160 -}
161 -
145 export function byteLengthOfChunk(chunk: Chunk | PrecomputedChunk): number {
146 return chunk.byteLength;
147 }
packages/react-server/src/ReactServerStreamConfigBun.js
-6
@@ -70,12 +70,6 @@ export function typedArrayToBinaryChunk(
70 return content;
71 }
72
73 -export function clonePrecomputedChunk(
74 - chunk: PrecomputedChunk,
75 -): PrecomputedChunk {
76 - return chunk;
77 -}
78 -
73 export function byteLengthOfChunk(chunk: Chunk | PrecomputedChunk): number {
74 return Buffer.byteLength(chunk, 'utf8');
75 }
packages/react-server/src/ReactServerStreamConfigEdge.js
+6 -23
@@ -22,7 +22,7 @@ export function flushBuffered(destination: Destination) {
22 // transform streams. https://github.com/whatwg/streams/issues/960
23 }
24
25 -const VIEW_SIZE = 512;
25 +const VIEW_SIZE = 2048;
26 let currentView = null;
27 let writtenBytes = 0;
28
@@ -40,15 +40,6 @@ export function writeChunk(
40 }
41
42 if (chunk.byteLength > VIEW_SIZE) {
43 - if (__DEV__) {
44 - if (precomputedChunkSet.has(chunk)) {
45 - console.error(
46 - 'A large precomputed chunk was passed to writeChunk without being copied.' +
47 - ' Large chunks get enqueued directly and are not copied. This is incompatible with precomputed chunks because you cannot enqueue the same precomputed chunk twice.' +
48 - ' Use "cloneChunk" to make a copy of this large precomputed chunk before writing it. This is a bug in React.',
49 - );
50 - }
51 - }
43 // this chunk may overflow a single view which implies it was not
44 // one that is cached by the streaming renderer. We will enqueu
45 // it directly and expect it is not re-used
@@ -120,15 +111,15 @@ export function stringToChunk(content: string): Chunk {
111 return textEncoder.encode(content);
112 }
113
123 -const precomputedChunkSet: Set<Chunk | BinaryChunk> = __DEV__
124 - ? new Set()
125 - : (null: any);
126 -
114 export function stringToPrecomputedChunk(content: string): PrecomputedChunk {
115 const precomputedChunk = textEncoder.encode(content);
116
117 if (__DEV__) {
131 - precomputedChunkSet.add(precomputedChunk);
118 + if (precomputedChunk.byteLength > VIEW_SIZE) {
119 + console.error(
120 + 'precomputed chunks must be smaller than the view size configured for this host. This is a bug in React.',
121 + );
122 + }
123 }
124
125 return precomputedChunk;
@@ -151,14 +142,6 @@ export function typedArrayToBinaryChunk(
142 return content.byteLength > VIEW_SIZE ? buffer.slice() : buffer;
143 }
144
154 -export function clonePrecomputedChunk(
155 - precomputedChunk: PrecomputedChunk,
156 -): PrecomputedChunk {
157 - return precomputedChunk.byteLength > VIEW_SIZE
158 - ? precomputedChunk.slice()
159 - : precomputedChunk;
160 -}
161 -
145 export function byteLengthOfChunk(chunk: Chunk | PrecomputedChunk): number {
146 return chunk.byteLength;
147 }
packages/react-server/src/ReactServerStreamConfigFB.js
-6
@@ -60,12 +60,6 @@ export function typedArrayToBinaryChunk(
60 throw new Error('Not implemented.');
61 }
62
63 -export function clonePrecomputedChunk(
64 - chunk: PrecomputedChunk,
65 -): PrecomputedChunk {
66 - return chunk;
67 -}
68 -
63 export function byteLengthOfChunk(chunk: Chunk | PrecomputedChunk): number {
64 throw new Error('Not implemented.');
65 }
packages/react-server/src/ReactServerStreamConfigNode.js
+4 -21
@@ -99,15 +99,6 @@ function writeViewChunk(
99 return;
100 }
101 if (chunk.byteLength > VIEW_SIZE) {
102 - if (__DEV__) {
103 - if (precomputedChunkSet && precomputedChunkSet.has(chunk)) {
104 - console.error(
105 - 'A large precomputed chunk was passed to writeChunk without being copied.' +
106 - ' Large chunks get enqueued directly and are not copied. This is incompatible with precomputed chunks because you cannot enqueue the same precomputed chunk twice.' +
107 - ' Use "cloneChunk" to make a copy of this large precomputed chunk before writing it. This is a bug in React.',
108 - );
109 - }
110 - }
102 // this chunk may overflow a single view which implies it was not
103 // one that is cached by the streaming renderer. We will enqueu
104 // it directly and expect it is not re-used
@@ -201,14 +192,14 @@ export function stringToChunk(content: string): Chunk {
192 return content;
193 }
194
204 -const precomputedChunkSet = __DEV__ ? new Set<PrecomputedChunk>() : null;
205 -
195 export function stringToPrecomputedChunk(content: string): PrecomputedChunk {
196 const precomputedChunk = textEncoder.encode(content);
197
198 if (__DEV__) {
210 - if (precomputedChunkSet) {
211 - precomputedChunkSet.add(precomputedChunk);
199 + if (precomputedChunk.byteLength > VIEW_SIZE) {
200 + console.error(
201 + 'precomputed chunks must be smaller than the view size configured for this host. This is a bug in React.',
202 + );
203 }
204 }
205
@@ -222,14 +213,6 @@ export function typedArrayToBinaryChunk(
213 return new Uint8Array(content.buffer, content.byteOffset, content.byteLength);
214 }
215
225 -export function clonePrecomputedChunk(
226 - precomputedChunk: PrecomputedChunk,
227 -): PrecomputedChunk {
228 - return precomputedChunk.length > VIEW_SIZE
229 - ? precomputedChunk.slice()
230 - : precomputedChunk;
231 -}
232 -
216 export function byteLengthOfChunk(chunk: Chunk | PrecomputedChunk): number {
217 return typeof chunk === 'string'
218 ? Buffer.byteLength(chunk, 'utf8')
packages/react-server/src/forks/ReactServerStreamConfig.custom.js
-1
@@ -41,7 +41,6 @@ export const closeWithError = $$$config.closeWithError;
41 export const stringToChunk = $$$config.stringToChunk;
42 export const stringToPrecomputedChunk = $$$config.stringToPrecomputedChunk;
43 export const typedArrayToBinaryChunk = $$$config.typedArrayToBinaryChunk;
44 -export const clonePrecomputedChunk = $$$config.clonePrecomputedChunk;
44 export const byteLengthOfChunk = $$$config.byteLengthOfChunk;
45 export const byteLengthOfBinaryChunk = $$$config.byteLengthOfBinaryChunk;
46 export const createFastHash = $$$config.createFastHash;