From a597c2f5dc5596bea331b455aca0548fb933038e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Wed, 14 Apr 2021 16:49:14 -0400 Subject: [PATCH] [Fizz] Fix reentrancy bug (#21270) * Fix reentrancy bug * Fix another reentrancy bug There's also an issue if we try to schedule something to be client rendered if its fallback hasn't rendered yet. So we don't do it in that case. --- .../src/__tests__/ReactDOMFizzServer-test.js | 17 ++++--- .../__tests__/ReactDOMFizzServerNode-test.js | 51 +++++++++++++++++-- packages/react-server/src/ReactFizzServer.js | 33 +++++++----- 3 files changed, 77 insertions(+), 24 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index edbb2e9f2e970..bcc4ad72a8b34 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -353,13 +353,16 @@ describe('ReactDOMFizzServer', () => { await act(async () => { const {startWriting} = ReactDOMFizzServer.pipeToNodeWritable( - }> - <> - -
- -
- + // We use two nested boundaries to flush out coverage of an old reentrancy bug. + + }> + <> + +
+ +
+ + , writableA, { diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js index c6a774fcf322e..dcef8c2406a6c 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js @@ -99,7 +99,7 @@ describe('ReactDOMFizzServer', () => { } return 'Done'; } - let isComplete = false; + let isCompleteCalls = 0; const {writable, output} = getTestWritable(); const {startWriting} = ReactDOMFizzServer.pipeToNodeWritable(
@@ -110,13 +110,13 @@ describe('ReactDOMFizzServer', () => { writable, { onCompleteAll() { - isComplete = true; + isCompleteCalls++; }, }, ); await jest.runAllTimers(); expect(output.result).toBe(''); - expect(isComplete).toBe(false); + expect(isCompleteCalls).toBe(0); // Resolve the loading. hasLoaded = true; await resolve(); @@ -124,7 +124,7 @@ describe('ReactDOMFizzServer', () => { await jest.runAllTimers(); expect(output.result).toBe(''); - expect(isComplete).toBe(true); + expect(isCompleteCalls).toBe(1); // First we write our header. output.result += @@ -244,6 +244,7 @@ describe('ReactDOMFizzServer', () => { // @gate experimental it('should be able to complete by aborting even if the promise never resolves', async () => { + let isCompleteCalls = 0; const {writable, output, completed} = getTestWritable(); const {startWriting, abort} = ReactDOMFizzServer.pipeToNodeWritable(
@@ -252,12 +253,53 @@ describe('ReactDOMFizzServer', () => {
, writable, + { + onCompleteAll() { + isCompleteCalls++; + }, + }, + ); + startWriting(); + + jest.runAllTimers(); + + expect(output.result).toContain('Loading'); + expect(isCompleteCalls).toBe(0); + + abort(); + + await completed; + + expect(output.error).toBe(undefined); + expect(output.result).toContain('Loading'); + expect(isCompleteCalls).toBe(1); + }); + + // @gate experimental + it('should be able to complete by abort when the fallback is also suspended', async () => { + let isCompleteCalls = 0; + const {writable, output, completed} = getTestWritable(); + const {startWriting, abort} = ReactDOMFizzServer.pipeToNodeWritable( +
+ + }> + + + +
, + writable, + { + onCompleteAll() { + isCompleteCalls++; + }, + }, ); startWriting(); jest.runAllTimers(); expect(output.result).toContain('Loading'); + expect(isCompleteCalls).toBe(0); abort(); @@ -265,5 +307,6 @@ describe('ReactDOMFizzServer', () => { expect(output.error).toBe(undefined); expect(output.result).toContain('Loading'); + expect(isCompleteCalls).toBe(1); }); }); diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index c1d51c3b2422e..4febbbe98163e 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -1172,8 +1172,8 @@ function abortTask(task: Task): void { const segment = task.blockedSegment; segment.status = ABORTED; - request.allPendingTasks--; if (boundary === null) { + request.allPendingTasks--; // We didn't complete the root so we have nothing to show. We can close // the request; if (request.status !== CLOSED) { @@ -1183,18 +1183,23 @@ function abortTask(task: Task): void { } else { boundary.pendingTasks--; - // If this boundary was still pending then we haven't already cancelled its fallbacks. - // We'll need to abort the fallbacks, which will also error that parent boundary. - boundary.fallbackAbortableTasks.forEach(abortTask, request); - boundary.fallbackAbortableTasks.clear(); - - if (!boundary.forceClientRender) { - boundary.forceClientRender = true; - if (boundary.parentFlushed) { - request.clientRenderedBoundaries.push(boundary); + if (boundary.fallbackAbortableTasks.size > 0) { + // If this boundary was still pending then we haven't already cancelled its fallbacks. + // We'll need to abort the fallbacks, which will also error that parent boundary. + // This means that we don't have to client render this boundary because its parent + // will be client rendered anyway. + boundary.fallbackAbortableTasks.forEach(abortTask, request); + boundary.fallbackAbortableTasks.clear(); + } else { + if (!boundary.forceClientRender) { + boundary.forceClientRender = true; + if (boundary.parentFlushed) { + request.clientRenderedBoundaries.push(boundary); + } } } + request.allPendingTasks--; if (request.allPendingTasks === 0) { const onCompleteAll = request.onCompleteAll; onCompleteAll(); @@ -1226,9 +1231,6 @@ function finishedTask( // This already errored. } else if (boundary.pendingTasks === 0) { // This must have been the last segment we were waiting on. This boundary is now complete. - // We can now cancel any pending task on the fallback since we won't need to show it anymore. - boundary.fallbackAbortableTasks.forEach(abortTaskSoft, request); - boundary.fallbackAbortableTasks.clear(); if (segment.parentFlushed) { // Our parent segment already flushed, so we need to schedule this segment to be emitted. boundary.completedSegments.push(segment); @@ -1238,6 +1240,11 @@ function finishedTask( // parent flushed, we need to schedule the boundary to be emitted. request.completedBoundaries.push(boundary); } + // We can now cancel any pending task on the fallback since we won't need to show it anymore. + // This needs to happen after we read the parentFlushed flags because aborting can finish + // work which can trigger user code, which can start flushing, which can change those flags. + boundary.fallbackAbortableTasks.forEach(abortTaskSoft, request); + boundary.fallbackAbortableTasks.clear(); } else { if (segment.parentFlushed) { // Our parent already flushed, so we need to schedule this segment to be emitted.