Re-add reentrancy avoidance (#23342)
* tests: add failing test to demonstrate bug in ReadableStream implementation * Re-add reentrancy avoidance I removed the equivalency of this in #22446. However, I didn't fully understand the intended semantics of the spec but I understand this better now. The spec is not actually recursive. It won't call pull again inside of a pull. It might not call it inside startWork neither which the original issue avoided. However, it will call pull if you enqueue to the controller without filling up the desired size outside any call. We could avoid that by returning a Promise from pull that we wait to resolve until we've performed all our pending tasks. That would be the more idiomatic solution. That's a bit more involved but since we know understand it, we can readd the reentrancy hack since we have an easy place to detect it. If anything, it should probably throw or log here otherwise. I believe this fixes #22772. This includes the test from #22889 but should ideally have one for Fizz. Co-authored-by: Josh Larson <josh.larson@shopify.com>
Sebastian Markbåge committed
Feb 23, 2022 at 00:33 UTC
2c693b2dee6e0a644e9e952b5c2b64a1b21c8e55
3 files changed
+151
-30
packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js
+143
-30
@@ -69,6 +69,31 @@ describe('ReactFlightDOMBrowser', () => {
69
}
70
}
71
72
+ function makeDelayedText(Model) {
73
+ let error, _resolve, _reject;
74
+ let promise = new Promise((resolve, reject) => {
75
+ _resolve = () => {
76
+ promise = null;
77
+ resolve();
78
+ };
79
+ _reject = e => {
80
+ error = e;
81
+ promise = null;
82
+ reject(e);
83
+ };
84
+ });
85
+ function DelayedText({children}, data) {
86
+ if (promise) {
87
+ throw promise;
88
+ }
89
+ if (error) {
90
+ throw error;
91
+ }
92
+ return <Model>{children}</Model>;
93
+ }
94
+ return [DelayedText, _resolve, _reject];
95
+ }
96
+
97
it('should resolve HTML using W3C streams', async () => {
98
function Text({children}) {
99
return <span>{children}</span>;
@@ -174,36 +199,11 @@ describe('ReactFlightDOMBrowser', () => {
199
return children;
200
}
201
177
- function makeDelayedText() {
178
- let error, _resolve, _reject;
179
- let promise = new Promise((resolve, reject) => {
180
- _resolve = () => {
181
- promise = null;
182
- resolve();
183
- };
184
- _reject = e => {
185
- error = e;
186
- promise = null;
187
- reject(e);
188
- };
189
- });
190
- function DelayedText({children}, data) {
191
- if (promise) {
192
- throw promise;
193
- }
194
- if (error) {
195
- throw error;
196
- }
197
- return <Text>{children}</Text>;
198
- }
199
- return [DelayedText, _resolve, _reject];
200
- }
201
-
202
- const [Friends, resolveFriends] = makeDelayedText();
203
- const [Name, resolveName] = makeDelayedText();
204
- const [Posts, resolvePosts] = makeDelayedText();
205
- const [Photos, resolvePhotos] = makeDelayedText();
206
- const [Games, , rejectGames] = makeDelayedText();
202
+ const [Friends, resolveFriends] = makeDelayedText(Text);
203
+ const [Name, resolveName] = makeDelayedText(Text);
204
+ const [Posts, resolvePosts] = makeDelayedText(Text);
205
+ const [Photos, resolvePhotos] = makeDelayedText(Text);
206
+ const [Games, , rejectGames] = makeDelayedText(Text);
207
208
// View
209
function ProfileDetails({avatar}) {
@@ -340,4 +340,117 @@ describe('ReactFlightDOMBrowser', () => {
340
341
expect(reportedErrors).toEqual([]);
342
});
343
+
344
+ it('should close the stream upon completion when rendering to W3C streams', async () => {
345
+ const {Suspense} = React;
346
+
347
+ // Model
348
+ function Text({children}) {
349
+ return children;
350
+ }
351
+
352
+ const [Friends, resolveFriends] = makeDelayedText(Text);
353
+ const [Name, resolveName] = makeDelayedText(Text);
354
+ const [Posts, resolvePosts] = makeDelayedText(Text);
355
+ const [Photos, resolvePhotos] = makeDelayedText(Text);
356
+
357
+ // View
358
+ function ProfileDetails({avatar}) {
359
+ return (
360
+ <div>
361
+ <Name>:name:</Name>
362
+ {avatar}
363
+ </div>
364
+ );
365
+ }
366
+ function ProfileSidebar({friends}) {
367
+ return (
368
+ <div>
369
+ <Photos>:photos:</Photos>
370
+ {friends}
371
+ </div>
372
+ );
373
+ }
374
+ function ProfilePosts({posts}) {
375
+ return <div>{posts}</div>;
376
+ }
377
+
378
+ function ProfileContent() {
379
+ return (
380
+ <Suspense fallback="(loading everything)">
381
+ <ProfileDetails avatar={<Text>:avatar:</Text>} />
382
+ <Suspense fallback={<p>(loading sidebar)</p>}>
383
+ <ProfileSidebar friends={<Friends>:friends:</Friends>} />
384
+ </Suspense>
385
+ <Suspense fallback={<p>(loading posts)</p>}>
386
+ <ProfilePosts posts={<Posts>:posts:</Posts>} />
387
+ </Suspense>
388
+ </Suspense>
389
+ );
390
+ }
391
+
392
+ const model = {
393
+ rootContent: <ProfileContent />,
394
+ };
395
+
396
+ const stream = ReactServerDOMWriter.renderToReadableStream(
397
+ model,
398
+ webpackMap,
399
+ );
400
+
401
+ const reader = stream.getReader();
402
+ const decoder = new TextDecoder();
403
+
404
+ let flightResponse = '';
405
+ let isDone = false;
406
+
407
+ reader.read().then(function progress({done, value}) {
408
+ if (done) {
409
+ isDone = true;
410
+ return;
411
+ }
412
+
413
+ flightResponse += decoder.decode(value);
414
+
415
+ return reader.read().then(progress);
416
+ });
417
+
418
+ // Advance time enough to trigger a nested fallback.
419
+ jest.advanceTimersByTime(500);
420
+
421
+ await act(async () => {});
422
+
423
+ expect(flightResponse).toContain('(loading everything)');
424
+ expect(flightResponse).toContain('(loading sidebar)');
425
+ expect(flightResponse).toContain('(loading posts)');
426
+ expect(flightResponse).not.toContain(':friends:');
427
+ expect(flightResponse).not.toContain(':name:');
428
+
429
+ await act(async () => {
430
+ resolveFriends();
431
+ });
432
+
433
+ expect(flightResponse).toContain(':friends:');
434
+
435
+ await act(async () => {
436
+ resolveName();
437
+ });
438
+
439
+ expect(flightResponse).toContain(':name:');
440
+
441
+ await act(async () => {
442
+ resolvePhotos();
443
+ });
444
+
445
+ expect(flightResponse).toContain(':photos:');
446
+
447
+ await act(async () => {
448
+ resolvePosts();
449
+ });
450
+
451
+ expect(flightResponse).toContain(':posts:');
452
+
453
+ // Final pending chunk is written; stream should be closed.
454
+ expect(isDone).toBeTruthy();
455
+ });
456
});
packages/react-server/src/ReactFizzServer.js
+4
@@ -1950,6 +1950,10 @@ export function startFlowing(request: Request, destination: Destination): void {
1950
if (request.status === CLOSED) {
1951
return;
1952
}
1953
+ if (request.destination !== null) {
1954
+ // We're already flowing.
1955
+ return;
1956
+ }
1957
request.destination = destination;
1958
try {
1959
flushCompletedQueues(request, destination);
packages/react-server/src/ReactFlightServer.js
+4
@@ -790,6 +790,10 @@ export function startFlowing(request: Request, destination: Destination): void {
790
if (request.status === CLOSED) {
791
return;
792
}
793
+ if (request.destination !== null) {
794
+ // We're already flowing.
795
+ return;
796
+ }
797
request.destination = destination;
798
try {
799
flushCompletedChunks(request, destination);