Skip to content

Commit

Permalink
Aborting early should not infinitely suspend
Browse files Browse the repository at this point in the history 
Before this change we weren't calling onError nor onFatalError if you abort
before completing the shell.

This means that the render never completes and hangs.

Aborting early can happen before even creating the stream for AbortSignal,
before rendering starts in Node since there's an setImmediate atm, or
during rendering.
  • Loading branch information
@sebmarkbage
sebmarkbage committed Jun 18, 2022
1 parent 72ebc70 commit e628d94
Show file tree
Hide file tree
Showing 4 changed files with 178 additions and 29 deletions.
113 changes: 109 additions & 4 deletions packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ let React;
let ReactDOMFizzServer;
let Suspense;

describe('ReactDOMFizzServer', () => {
describe('ReactDOMFizzServerBrowser', () => {
beforeEach(() => {
jest.resetModules();
React = require('react');
Expand Down Expand Up @@ -209,6 +209,111 @@ describe('ReactDOMFizzServer', () => {
]);
});

it('should reject if aborting before the shell is complete', async () => {
const errors = [];
const controller = new AbortController();
const promise = ReactDOMFizzServer.renderToReadableStream(
<div>
<InfiniteSuspend />
</div>,
{
signal: controller.signal,
onError(x) {
errors.push(x.message);
},
},
);

await jest.runAllTimers();

const theReason = new Error('aborted for reasons');
// @TODO this is a hack to work around lack of support for abortSignal.reason in node
// The abort call itself should set this property but since we are testing in node we
// set it here manually
controller.signal.reason = theReason;
controller.abort(theReason);

let caughtError = null;
try {
await promise;
} catch (error) {
caughtError = error;
}
expect(caughtError).toBe(theReason);
expect(errors).toEqual(['aborted for reasons']);
});

it('should be able to abort before something suspends', async () => {
const errors = [];
const controller = new AbortController();
function App() {
controller.abort();
return (
<Suspense fallback={<div>Loading</div>}>
<InfiniteSuspend />
</Suspense>
);
}
const streamPromise = ReactDOMFizzServer.renderToReadableStream(
<div>
<App />
</div>,
{
signal: controller.signal,
onError(x) {
errors.push(x.message);
},
},
);

let caughtError = null;
try {
await streamPromise;
} catch (error) {
caughtError = error;
}
expect(caughtError.message).toBe('The render was aborted by the server without a reason.');
expect(errors).toEqual([
'The render was aborted by the server without a reason.',
]);
});

it('should reject if passing an already aborted signal', async () => {
const errors = [];
const controller = new AbortController();
const theReason = new Error('aborted for reasons');
// @TODO this is a hack to work around lack of support for abortSignal.reason in node
// The abort call itself should set this property but since we are testing in node we
// set it here manually
controller.signal.reason = theReason;
controller.abort(theReason);

const promise = ReactDOMFizzServer.renderToReadableStream(
<div>
<Suspense fallback={<div>Loading</div>}>
<InfiniteSuspend />
</Suspense>
</div>,
{
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 caughtError = null;
try {
await promise;
} catch (error) {
caughtError = error;
}
expect(caughtError).toBe(theReason);
expect(errors).toEqual(['aborted for reasons']);
});

it('should not continue rendering after the reader cancels', async () => {
let hasLoaded = false;
let resolve;
Expand All @@ -226,7 +331,7 @@ describe('ReactDOMFizzServer', () => {
const stream = await ReactDOMFizzServer.renderToReadableStream(
<div>
<Suspense fallback={<div>Loading</div>}>
<Wait /> />
<Wait />
</Suspense>
</div>,
{
Expand Down Expand Up @@ -296,7 +401,7 @@ describe('ReactDOMFizzServer', () => {
expect(result).toMatchInlineSnapshot(`"<div>${str2049}</div>"`);
});

it('Supports custom abort reasons with a string', async () => {
it('supports custom abort reasons with a string', async () => {
const promise = new Promise(r => {});
function Wait() {
throw promise;
Expand Down Expand Up @@ -337,7 +442,7 @@ describe('ReactDOMFizzServer', () => {
expect(errors).toEqual(['foobar', 'foobar']);
});

it('Supports custom abort reasons with an Error', async () => {
it('supports custom abort reasons with an Error', async () => {
const promise = new Promise(r => {});
function Wait() {
throw promise;
Expand Down
49 changes: 44 additions & 5 deletions packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ let React;
let ReactDOMFizzServer;
let Suspense;

describe('ReactDOMFizzServer', () => {
describe('ReactDOMFizzServerNode', () => {
beforeEach(() => {
jest.resetModules();
React = require('react');
Expand Down Expand Up @@ -166,7 +166,6 @@ describe('ReactDOMFizzServer', () => {
<div>
<Throw />
</div>,

{
onError(x) {
reportedErrors.push(x);
Expand Down Expand Up @@ -232,7 +231,6 @@ describe('ReactDOMFizzServer', () => {
<Throw />
</Suspense>
</div>,

{
onError(x) {
reportedErrors.push(x);
Expand Down Expand Up @@ -288,7 +286,6 @@ describe('ReactDOMFizzServer', () => {
<InfiniteSuspend />
</Suspense>
</div>,

{
onError(x) {
errors.push(x.message);
Expand All @@ -315,6 +312,49 @@ describe('ReactDOMFizzServer', () => {
expect(isCompleteCalls).toBe(1);
});

it('should fail the shell if you abort before work has begun', async () => {
let isCompleteCalls = 0;
const errors = [];
const shellErrors = [];
const {writable, output, completed} = getTestWritable();
const {pipe, abort} = ReactDOMFizzServer.renderToPipeableStream(
<div>
<Suspense fallback={<div>Loading</div>}>
<InfiniteSuspend />
</Suspense>
</div>,
{
onError(x) {
errors.push(x.message);
},
onShellError(x) {
shellErrors.push(x.message);
},
onAllReady() {
isCompleteCalls++;
},
},
);
pipe(writable);

// Currently we delay work so if we abort, we abort the remaining CPU
// work as well.

// Abort before running the timers that perform the work
const theReason = new Error('uh oh');
abort(theReason);

jest.runAllTimers();

await completed;

expect(errors).toEqual(['uh oh']);
expect(shellErrors).toEqual(['uh oh']);
expect(output.error).toBe(theReason);
expect(output.result).toBe('');
expect(isCompleteCalls).toBe(0);
});

it('should be able to complete by abort when the fallback is also suspended', async () => {
let isCompleteCalls = 0;
const errors = [];
Expand All @@ -327,7 +367,6 @@ describe('ReactDOMFizzServer', () => {
</Suspense>
</Suspense>
</div>,

{
onError(x) {
errors.push(x.message);
Expand Down
12 changes: 8 additions & 4 deletions packages/react-dom/src/server/ReactDOMFizzServerBrowser.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,15 @@ function renderToReadableStream(
);
if (options && options.signal) {
const signal = options.signal;
const listener = () => {
if (signal.aborted) {
abort(request, (signal: any).reason);
signal.removeEventListener('abort', listener);
};
signal.addEventListener('abort', listener);
} else {
const listener = () => {
abort(request, (signal: any).reason);
signal.removeEventListener('abort', listener);
};
signal.addEventListener('abort', listener);
}
}
startWork(request);
});
Expand Down
33 changes: 17 additions & 16 deletions packages/react-server/src/ReactFizzServer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1530,7 +1530,7 @@ function abortTaskSoft(task: Task): void {
finishedTask(request, boundary, segment);
}

function abortTask(task: Task, request: Request, reason: mixed): void {
function abortTask(task: Task, request: Request, error: mixed): void {
// This aborts the task and aborts the parent that it blocks, putting it into
// client rendered mode.
const boundary = task.blockedBoundary;
Expand All @@ -1541,35 +1541,30 @@ function abortTask(task: Task, request: Request, reason: mixed): void {
request.allPendingTasks--;
// We didn't complete the root so we have nothing to show. We can close
// the request;
if (request.status !== CLOSED) {
request.status = CLOSED;
if (request.destination !== null) {
close(request.destination);
}
if (request.status !== CLOSING && request.status !== CLOSED) {
logRecoverableError(request, error);
fatalError(request, error);
}
} else {
boundary.pendingTasks--;

if (!boundary.forceClientRender) {
boundary.forceClientRender = true;
let error =
reason === undefined
? new Error('The render was aborted by the server without a reason.')
: reason;
boundary.errorDigest = request.onError(error);
if (__DEV__) {
const errorPrefix =
'The server did not finish this Suspense boundary: ';
let errorMessage;
if (error && typeof error.message === 'string') {
error = errorPrefix + error.message;
errorMessage = errorPrefix + error.message;
} else {
// eslint-disable-next-line react-internal/safe-string-coercion
error = errorPrefix + String(error);
errorMessage = errorPrefix + String(error);
}
const previousTaskInDev = currentTaskInDEV;
currentTaskInDEV = task;
try {
captureBoundaryErrorDetailsDev(boundary, error);
captureBoundaryErrorDetailsDev(boundary, errorMessage);
} finally {
currentTaskInDEV = previousTaskInDev;
}
Expand All @@ -1582,7 +1577,7 @@ function abortTask(task: Task, request: Request, reason: mixed): void {
// 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, reason),
abortTask(fallbackTask, request, error),
);
boundary.fallbackAbortableTasks.clear();

Expand Down Expand Up @@ -2178,8 +2173,14 @@ export function startFlowing(request: Request, destination: Destination): void {
export function abort(request: Request, reason: mixed): void {
try {
const abortableTasks = request.abortableTasks;
abortableTasks.forEach(task => abortTask(task, request, reason));
abortableTasks.clear();
if (abortableTasks.size > 0) {
const error =
reason === undefined
? new Error('The render was aborted by the server without a reason.')
: reason;
abortableTasks.forEach(task => abortTask(task, request, error));
abortableTasks.clear();
}
if (request.destination !== null) {
flushCompletedQueues(request, request.destination);
}
Expand Down

0 comments on commit e628d94

Please sign in to comment.