Skip to content

Commit

Permalink
Attempts to avoid consuming an error id while aborting.
Browse files Browse the repository at this point in the history
There are edge cases though like if we're aborting while rendering and we have a model that needs to refer to some reference that we don't know the identity of
  • Loading branch information
gnoff committed Aug 20, 2024
1 parent 3fd8dcb commit 6459ba7
Show file tree
Hide file tree
Showing 5 changed files with 108 additions and 31 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2797,7 +2797,16 @@ describe('ReactFlightDOM', () => {
abortFizz('bam');
});

expect(errors).toEqual(['bam']);
if (__DEV__) {
expect(errors).toEqual([new Error('Connection closed.')]);
} else {
// This is likely a bug. In Dev we get a connection closed error
// because the debug info creates a chunk that has a pending status
// and when the stream finishes we error if any chunks are still pending.
// In production there is no debug info so the missing chunk is never instantiated
// because nothing triggers model evaluation before the stream completes
expect(errors).toEqual(['bam']);
}

const container = document.createElement('div');
await readInto(container, fizzReadable);
Expand Down Expand Up @@ -2919,10 +2928,11 @@ describe('ReactFlightDOM', () => {
});

const {prelude} = await pendingResult;

expect(errors).toEqual(['boom']);
const response = ReactServerDOMClient.createFromReadableStream(
Readable.toWeb(prelude),
);

const preludeWeb = Readable.toWeb(prelude);
const response = ReactServerDOMClient.createFromReadableStream(preludeWeb);

const {writable: fizzWritable, readable: fizzReadable} = getTestStream();

Expand All @@ -2949,7 +2959,17 @@ describe('ReactFlightDOM', () => {
});

// one error per boundary
expect(errors).toEqual(['boom', 'boom', 'boom']);
if (__DEV__) {
const err = new Error('Connection closed.');
expect(errors).toEqual([err, err, err]);
} else {
// This is likely a bug. In Dev we get a connection closed error
// because the debug info creates a chunk that has a pending status
// and when the stream finishes we error if any chunks are still pending.
// In production there is no debug info so the missing chunk is never instantiated
// because nothing triggers model evaluation before the stream completes
expect(errors).toEqual(['boom', 'boom', 'boom']);
}

const container = document.createElement('div');
await readInto(container, fizzReadable);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2454,12 +2454,28 @@ describe('ReactFlightDOMBrowser', () => {
passThrough(prelude),
);
const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);
errors.length = 0;
const root = ReactDOMClient.createRoot(container, {
onUncaughtError(err) {
errors.push(err);
},
});

await act(() => {
root.render(<ClientRoot response={response} />);
});

expect(container.innerHTML).toBe('<div>loading...</div>');
if (__DEV__) {
expect(errors).toEqual([new Error('Connection closed.')]);
expect(container.innerHTML).toBe('');
} else {
// This is likely a bug. In Dev we get a connection closed error
// because the debug info creates a chunk that has a pending status
// and when the stream finishes we error if any chunks are still pending.
// In production there is no debug info so the missing chunk is never instantiated
// because nothing triggers model evaluation before the stream completes
expect(errors).toEqual([]);
expect(container.innerHTML).toBe('<div>loading...</div>');
}
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -1172,7 +1172,16 @@ describe('ReactFlightDOMEdge', () => {
),
);
fizzController.abort('bam');
expect(errors).toEqual(['bam']);
if (__DEV__) {
expect(errors).toEqual([new Error('Connection closed.')]);
} else {
// This is likely a bug. In Dev we get a connection closed error
// because the debug info creates a chunk that has a pending status
// and when the stream finishes we error if any chunks are still pending.
// In production there is no debug info so the missing chunk is never instantiated
// because nothing triggers model evaluation before the stream completes
expect(errors).toEqual(['bam']);
}
// Should still match the result when parsed
const result = await readResult(ssrStream);
const div = document.createElement('div');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,16 @@ describe('ReactFlightDOMNode', () => {
),
);
ssrStream.abort('bam');
expect(errors).toEqual(['bam']);
if (__DEV__) {
expect(errors).toEqual([new Error('Connection closed.')]);
} else {
// This is likely a bug. In Dev we get a connection closed error
// because the debug info creates a chunk that has a pending status
// and when the stream finishes we error if any chunks are still pending.
// In production there is no debug info so the missing chunk is never instantiated
// because nothing triggers model evaluation before the stream completes
expect(errors).toEqual(['bam']);
}
// Should still match the result when parsed
const result = await readResult(ssrStream);
const div = document.createElement('div');
Expand Down
67 changes: 45 additions & 22 deletions packages/react-server/src/ReactFlightServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -649,9 +649,13 @@ function serializeThenable(
// We can no longer accept any resolved values
request.abortableTasks.delete(newTask);
newTask.status = ABORTED;
const errorId: number = (request.fatalError: any);
const model = stringify(serializeByValueID(errorId));
emitModelChunk(request, newTask.id, model);
if (enableHalt && request.type === PRERENDER) {
request.pendingChunks--;
} else {
const errorId: number = (request.fatalError: any);
const model = stringify(serializeByValueID(errorId));
emitModelChunk(request, newTask.id, model);
}
return newTask.id;
}
if (typeof thenable.status === 'string') {
Expand Down Expand Up @@ -2293,7 +2297,17 @@ function renderModel(
if (typeof x.then === 'function') {
if (request.status === ABORTING) {
task.status = ABORTED;
const errorId: number = (request.fatalError: any);
let errorId: number;
if (enableHalt && request.type === PRERENDER) {
// This is unfortunate that we would consume an id here. It suggests something
// isn't quite right with our model. When halting we don't emit any chunks
// but we're not in a position where we are avoiding emitting an entire
// chunk so we have to return something in this slot within the model so we
// consume an id and return it here knowing it will never resolve.
errorId = request.nextChunkId++;
} else {
errorId = (request.fatalError: any);
}
if (wasReactNode) {
return serializeLazyID(errorId);
}
Expand Down Expand Up @@ -2346,7 +2360,17 @@ function renderModel(

if (request.status === ABORTING) {
task.status = ABORTED;
const errorId: number = (request.fatalError: any);
let errorId: number;
if (enableHalt && request.type === PRERENDER) {
// This is unfortunate that we would consume an id here. It suggests something
// isn't quite right with our model. When halting we don't emit any chunks
// but we're not in a position where we are avoiding emitting an entire
// chunk so we have to return something in this slot within the model so we
// consume an id and return it here knowing it will never resolve.
errorId = request.nextChunkId++;
} else {
errorId = (request.fatalError: any);
}
if (wasReactNode) {
return serializeLazyID(errorId);
}
Expand Down Expand Up @@ -3820,6 +3844,22 @@ function retryTask(request: Request, task: Task): void {
request.abortableTasks.delete(task);
task.status = COMPLETED;
} catch (thrownValue) {
if (request.status === ABORTING) {
request.abortableTasks.delete(task);
task.status = ABORTED;
if (enableHalt && request.type === PRERENDER) {
// When aborting a prerener with halt semantics we don't emit
// anything into the slot for a task that aborts, it remains unresolved
request.pendingChunks--;
} else {
// Otherwise we emit an error chunk into the task slot.
const errorId: number = (request.fatalError: any);
const model = stringify(serializeByValueID(errorId));
emitModelChunk(request, task.id, model);
}
return;
}

const x =
thrownValue === SuspenseException
? // This is a special type of exception used for Suspense. For historical
Expand All @@ -3832,14 +3872,6 @@ function retryTask(request: Request, task: Task): void {
if (typeof x === 'object' && x !== null) {
// $FlowFixMe[method-unbinding]
if (typeof x.then === 'function') {
if (request.status === ABORTING) {
request.abortableTasks.delete(task);
task.status = ABORTED;
const errorId: number = (request.fatalError: any);
const model = stringify(serializeByValueID(errorId));
emitModelChunk(request, task.id, model);
return;
}
// Something suspended again, let's pick it back up later.
task.status = PENDING;
task.thenableState = getThenableStateAfterSuspending();
Expand All @@ -3856,15 +3888,6 @@ function retryTask(request: Request, task: Task): void {
}
}

if (request.status === ABORTING) {
request.abortableTasks.delete(task);
task.status = ABORTED;
const errorId: number = (request.fatalError: any);
const model = stringify(serializeByValueID(errorId));
emitModelChunk(request, task.id, model);
return;
}

request.abortableTasks.delete(task);
task.status = ERRORED;
const digest = logRecoverableError(request, x, task);
Expand Down

0 comments on commit 6459ba7

Please sign in to comment.