From bf19314810230862786745b9e1f622c5aff7c48a Mon Sep 17 00:00:00 2001 From: Josh Story Date: Sat, 17 Aug 2024 14:21:05 -0700 Subject: [PATCH] [Fizz][Static] when aborting a prerender halt unfinished boundaries instead of erroring When we introduces prerendering for flight we modeled an abort of a flight prerender as having unfinished rows. This is similar to how postpone was already implemented when you postponed from "within" a prerender using React.unstable_postpone. However when aborting with a postponed instance every boundary would be eagerly marked for client rendering which is more akin to prerendering and then resuming with an aborted signal. The insight with the flight work was that it's not so much the postpone that describes the intended semantics but the abort combined with a prerender. So like in flight when you abort a prerender and enableHalt is enabled boundaries and the shell won't error for any reason. Fizz will still call onPostpone and onError according to the abort reason but the consuemr of the prerender should expect to resume it before trying to use it. --- .../src/__tests__/ReactDOMFizzServer-test.js | 106 ++++++++++++++++++ .../src/__tests__/ReactDOMFizzStatic-test.js | 52 +++++++++ .../ReactDOMFizzStaticBrowser-test.js | 101 +++++++++++++++-- .../__tests__/ReactDOMFizzStaticNode-test.js | 98 ++++++++++++++-- packages/react-server/src/ReactFizzServer.js | 88 ++++++++++++++- 5 files changed, 424 insertions(+), 21 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index 7d8707bcd3f22..5a763ffe949ab 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -7746,6 +7746,112 @@ describe('ReactDOMFizzServer', () => { ); }); + // @gate enableHalt + it('can resume a prerender that was aborted', async () => { + const promise = new Promise(r => {}); + + let prerendering = true; + + function Wait() { + if (prerendering) { + return React.use(promise); + } else { + return 'Hello'; + } + } + + function App() { + return ( +
+ +

+ + + + + +

+

+ + + + + +

+ +
+ ); + } + + const controller = new AbortController(); + const signal = controller.signal; + + const errors = []; + function onError(error) { + errors.push(error); + } + let pendingPrerender; + await act(() => { + pendingPrerender = ReactDOMFizzStatic.prerenderToNodeStream(, { + signal, + onError, + }); + }); + controller.abort('boom'); + + const prerendered = await pendingPrerender; + + expect(errors).toEqual(['boom', 'boom']); + + const preludeWritable = new Stream.PassThrough(); + preludeWritable.setEncoding('utf8'); + preludeWritable.on('data', chunk => { + writable.write(chunk); + }); + + await act(() => { + prerendered.prelude.pipe(preludeWritable); + }); + + expect(getVisibleChildren(container)).toEqual( +
+

+ Loading again... +

+

+ Loading again too... +

+
, + ); + + prerendering = false; + + errors.length = 0; + const resumed = await ReactDOMFizzServer.resumeToPipeableStream( + , + JSON.parse(JSON.stringify(prerendered.postponed)), + { + onError, + }, + ); + + await act(() => { + resumed.pipe(writable); + }); + + expect(errors).toEqual([]); + expect(getVisibleChildren(container)).toEqual( +
+

+ Hello +

+

+ Hello +

+
, + ); + }); + // @gate enablePostpone it('does not call onError when you abort with a postpone instance during resume', async () => { let prerendering = true; diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzStatic-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzStatic-test.js index 1f3bfa7b3308d..03db4e3f5ed8f 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzStatic-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzStatic-test.js @@ -454,4 +454,56 @@ describe('ReactDOMFizzStatic', () => { }); expect(getVisibleChildren(container)).toEqual(undefined); }); + + // @gate enableHalt + it('will halt a prerender when aborting with an error during a render', async () => { + const controller = new AbortController(); + function App() { + controller.abort('sync'); + return
hello world
; + } + + const errors = []; + const result = await ReactDOMFizzStatic.prerenderToNodeStream(, { + signal: controller.signal, + onError(error) { + errors.push(error); + }, + }); + await act(async () => { + result.prelude.pipe(writable); + }); + expect(errors).toEqual(['sync']); + expect(getVisibleChildren(container)).toEqual(undefined); + }); + + // @gate enableHalt + it('will halt a prerender when aborting with an error in a microtask', async () => { + const errors = []; + + const controller = new AbortController(); + function App() { + React.use( + new Promise(() => { + Promise.resolve().then(() => { + controller.abort('async'); + }); + }), + ); + return
hello world
; + } + + errors.length = 0; + const result = await ReactDOMFizzStatic.prerenderToNodeStream(, { + signal: controller.signal, + onError(error) { + errors.push(error); + }, + }); + await act(async () => { + result.prelude.pipe(writable); + }); + expect(errors).toEqual(['async']); + expect(getVisibleChildren(container)).toEqual(undefined); + }); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js index 7a3db48b016e3..357ef1dcb478e 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js @@ -307,7 +307,8 @@ describe('ReactDOMFizzStaticBrowser', () => { }); // @gate experimental - it('should reject if aborting before the shell is complete', async () => { + // @gate !enableHalt + it('should reject if aborting before the shell is complete and enableHalt is disabled', async () => { const errors = []; const controller = new AbortController(); const promise = serverAct(() => @@ -339,6 +340,42 @@ describe('ReactDOMFizzStaticBrowser', () => { expect(errors).toEqual(['aborted for reasons']); }); + // @gate enableHalt + it('should resolve an empty prelude if aborting before the shell is complete', async () => { + const errors = []; + const controller = new AbortController(); + const promise = serverAct(() => + ReactDOMFizzStatic.prerender( +
+ +
, + { + signal: controller.signal, + onError(x) { + errors.push(x.message); + }, + }, + ), + ); + + await jest.runAllTimers(); + + const theReason = new Error('aborted for reasons'); + controller.abort(theReason); + + let rejected = false; + let prelude; + try { + ({prelude} = await promise); + } catch (error) { + rejected = true; + } + expect(rejected).toBe(false); + expect(errors).toEqual(['aborted for reasons']); + const content = await readContent(prelude); + expect(content).toBe(''); + }); + // @gate experimental it('should be able to abort before something suspends', async () => { const errors = []; @@ -365,18 +402,26 @@ describe('ReactDOMFizzStaticBrowser', () => { ), ); - let caughtError = null; - try { - await streamPromise; - } catch (error) { - caughtError = error; + if (gate(flags => flags.enableHalt)) { + const {prelude} = await streamPromise; + const content = await readContent(prelude); + expect(errors).toEqual(['The operation was aborted.']); + expect(content).toBe(''); + } else { + let caughtError = null; + try { + await streamPromise; + } catch (error) { + caughtError = error; + } + expect(caughtError.message).toBe('The operation was aborted.'); + expect(errors).toEqual(['The operation was aborted.']); } - expect(caughtError.message).toBe('The operation was aborted.'); - expect(errors).toEqual(['The operation was aborted.']); }); // @gate experimental - it('should reject if passing an already aborted signal', async () => { + // @gate !enableHalt + it('should reject if passing an already aborted signal and enableHalt is disabled', async () => { const errors = []; const controller = new AbortController(); const theReason = new Error('aborted for reasons'); @@ -410,6 +455,44 @@ describe('ReactDOMFizzStaticBrowser', () => { expect(errors).toEqual(['aborted for reasons']); }); + // @gate enableHalt + it('should resolve an empty prelude if passing an already aborted signal', async () => { + const errors = []; + const controller = new AbortController(); + const theReason = new Error('aborted for reasons'); + controller.abort(theReason); + + const promise = serverAct(() => + ReactDOMFizzStatic.prerender( +
+ Loading
}> + + + , + { + signal: controller.signal, + onError(x) { + errors.push(x.message); + }, + }, + ), + ); + + // Technically we could still continue rendering the shell but currently the + // semantics mean that we also abort any pending CPU work. + let didThrow = false; + let prelude; + try { + ({prelude} = await promise); + } catch (error) { + didThrow = true; + } + expect(didThrow).toBe(false); + expect(errors).toEqual(['aborted for reasons']); + const content = await readContent(prelude); + expect(content).toBe(''); + }); + // @gate experimental it('supports custom abort reasons with a string', async () => { const promise = new Promise(r => {}); diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzStaticNode-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzStaticNode-test.js index ade755bdffea1..12ac4de34d684 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzStaticNode-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzStaticNode-test.js @@ -212,7 +212,8 @@ describe('ReactDOMFizzStaticNode', () => { }); // @gate experimental - it('should reject if aborting before the shell is complete', async () => { + // @gate !enableHalt + it('should reject if aborting before the shell is complete and enableHalt is disabled', async () => { const errors = []; const controller = new AbortController(); const promise = ReactDOMFizzStatic.prerenderToNodeStream( @@ -242,6 +243,40 @@ describe('ReactDOMFizzStaticNode', () => { expect(errors).toEqual(['aborted for reasons']); }); + // @gate enableHalt + it('should resolve an empty shell if aborting before the shell is complete', async () => { + const errors = []; + const controller = new AbortController(); + const promise = ReactDOMFizzStatic.prerenderToNodeStream( +
+ +
, + { + signal: controller.signal, + onError(x) { + errors.push(x.message); + }, + }, + ); + + await jest.runAllTimers(); + + const theReason = new Error('aborted for reasons'); + controller.abort(theReason); + + let didThrow = false; + let prelude; + try { + ({prelude} = await promise); + } catch (error) { + didThrow = true; + } + expect(didThrow).toBe(false); + expect(errors).toEqual(['aborted for reasons']); + const content = await readContent(prelude); + expect(content).toBe(''); + }); + // @gate experimental it('should be able to abort before something suspends', async () => { const errors = []; @@ -266,18 +301,26 @@ describe('ReactDOMFizzStaticNode', () => { }, ); - let caughtError = null; - try { - await streamPromise; - } catch (error) { - caughtError = error; + if (gate(flags => flags.enableHalt)) { + const {prelude} = await streamPromise; + const content = await readContent(prelude); + expect(errors).toEqual(['This operation was aborted']); + expect(content).toBe(''); + } else { + let caughtError = null; + try { + await streamPromise; + } catch (error) { + caughtError = error; + } + expect(caughtError.message).toBe('This operation was aborted'); + expect(errors).toEqual(['This operation was aborted']); } - expect(caughtError.message).toBe('This operation was aborted'); - expect(errors).toEqual(['This operation was aborted']); }); // @gate experimental - it('should reject if passing an already aborted signal', async () => { + // @gate !enableHalt + it('should reject if passing an already aborted signal and enableHalt is disabled', async () => { const errors = []; const controller = new AbortController(); const theReason = new Error('aborted for reasons'); @@ -309,6 +352,43 @@ describe('ReactDOMFizzStaticNode', () => { expect(errors).toEqual(['aborted for reasons']); }); + // @gate enableHalt + it('should resolve with an empty prelude if passing an already aborted signal', async () => { + const errors = []; + const controller = new AbortController(); + const theReason = new Error('aborted for reasons'); + controller.abort(theReason); + + const promise = ReactDOMFizzStatic.prerenderToNodeStream( +
+ Loading
}> + + + , + { + signal: controller.signal, + onError(x) { + errors.push(x.message); + }, + }, + ); + + // Technically we could still continue rendering the shell but currently the + // semantics mean that we also abort any pending CPU work. + + let didThrow = false; + let prelude; + try { + ({prelude} = await promise); + } catch (error) { + didThrow = true; + } + expect(didThrow).toBe(false); + expect(errors).toEqual(['aborted for reasons']); + const content = await readContent(prelude); + expect(content).toBe(''); + }); + // @gate experimental it('supports custom abort reasons with a string', async () => { const promise = new Promise(r => {}); diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index 17c278f2b0b52..daea492db5b8e 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -157,6 +157,7 @@ import { enableSuspenseAvoidThisFallbackFizz, enableCache, enablePostpone, + enableHalt, enableRenderableContext, enableRefAsProp, disableDefaultPropsExceptForClasses, @@ -3625,6 +3626,9 @@ function erroredTask( ) { // Report the error to a global handler. let errorDigest; + // We don't handle halts here because we only halt when prerendering and + // when prerendering we should be finishing tasks not erroring them when + // they halt or postpone if ( enablePostpone && typeof error === 'object' && @@ -3812,6 +3816,17 @@ function abortTask(task: Task, request: Request, error: mixed): void { logRecoverableError(request, fatal, errorInfo, null); fatalError(request, fatal, errorInfo, null); } + } else if ( + enableHalt && + request.trackedPostpones !== null && + segment !== null + ) { + const trackedPostpones = request.trackedPostpones; + // We are aborting a prerender and must treat the shell as halted + // We log the error but we still resolve the prerender + logRecoverableError(request, error, errorInfo, null); + trackPostpone(request, trackedPostpones, task, segment); + finishedTask(request, null, segment); } else { logRecoverableError(request, error, errorInfo, null); fatalError(request, error, errorInfo, null); @@ -3856,10 +3871,40 @@ function abortTask(task: Task, request: Request, error: mixed): void { } } else { boundary.pendingTasks--; + // We construct an errorInfo from the boundary's componentStack so the error in dev will indicate which + // boundary the message is referring to + const errorInfo = getThrownInfo(task.componentStack); + const trackedPostpones = request.trackedPostpones; if (boundary.status !== CLIENT_RENDERED) { - // We construct an errorInfo from the boundary's componentStack so the error in dev will indicate which - // boundary the message is referring to - const errorInfo = getThrownInfo(task.componentStack); + if (enableHalt) { + if (trackedPostpones !== null && segment !== null) { + // We are aborting a prerender + if ( + enablePostpone && + typeof error === 'object' && + error !== null && + error.$$typeof === REACT_POSTPONE_TYPE + ) { + const postponeInstance: Postpone = (error: any); + logPostpone(request, postponeInstance.message, errorInfo, null); + } else { + // We are aborting a prerender and must halt this boundary. + // We treat this like other postpones during prerendering + logRecoverableError(request, error, errorInfo, null); + } + trackPostpone(request, trackedPostpones, task, segment); + // 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(fallbackTask => + abortTask(fallbackTask, request, error), + ); + boundary.fallbackAbortableTasks.clear(); + return finishedTask(request, boundary, segment); + } + } + boundary.status = CLIENT_RENDERED; + // We are aborting a render or resume which should put boundaries + // into an explicitly client rendered state let errorDigest; if ( enablePostpone && @@ -4145,6 +4190,43 @@ function retryRenderTask( ? request.fatalError : thrownValue; + if ( + enableHalt && + request.status === ABORTING && + request.trackedPostpones !== null + ) { + // We are aborting a prerender and need to halt this task. + const trackedPostpones = request.trackedPostpones; + const thrownInfo = getThrownInfo(task.componentStack); + task.abortSet.delete(task); + + if ( + enablePostpone && + typeof x === 'object' && + x !== null && + x.$$typeof === REACT_POSTPONE_TYPE + ) { + const postponeInstance: Postpone = (x: any); + logPostpone( + request, + postponeInstance.message, + thrownInfo, + __DEV__ && enableOwnerStacks ? task.debugTask : null, + ); + } else { + logRecoverableError( + request, + x, + thrownInfo, + __DEV__ && enableOwnerStacks ? task.debugTask : null, + ); + } + + trackPostpone(request, trackedPostpones, task, segment); + finishedTask(request, task.blockedBoundary, segment); + return; + } + if (typeof x === 'object' && x !== null) { // $FlowFixMe[method-unbinding] if (typeof x.then === 'function') {