Skip to content

Commit

Permalink
Treat blocked modules or models as a special status
Browse files Browse the repository at this point in the history 
We currently loop over all chunks at the end to error them if they're
still pending. We shouldn't do this if they're pending because they're
blocked on an external resource like a module because the module might not
resolve before the Flight connection closes and that's not an error.

In an alternative solution I had a set that tracked pending chunks and
removed one at a time. While the loop at the end is faster it's more
work as we go.

I figured the extra status might also help debugging.

For modules we can probably assume no forward references, and the first
async module we can just use the promise as the chunk.

So we could probably get away with this only on models that are blocked by
modules.
  • Loading branch information
@sebmarkbage
sebmarkbage committed Sep 15, 2022
1 parent b41cf4f commit 1c481e9
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 44 deletions.
59 changes: 43 additions & 16 deletions packages/react-client/src/ReactFlightClient.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export type JSONValue =
| $ReadOnlyArray<JSONValue>;

const PENDING = 'pending';
const BLOCKED = 'blocked';
const RESOLVED_MODEL = 'resolved_model';
const RESOLVED_MODULE = 'resolved_module';
const INITIALIZED = 'fulfilled';
Expand All @@ -50,6 +51,13 @@ type PendingChunk<T> = {
_response: Response,
then(resolve: (T) => mixed, reject: (mixed) => mixed): void,
};
type BlockedChunk<T> = {
status: 'blocked',
value: null | Array<(T) => mixed>,
reason: null | Array<(mixed) => mixed>,
_response: Response,
then(resolve: (T) => mixed, reject: (mixed) => mixed): void,
};
type ResolvedModelChunk<T> = {
status: 'resolved_model',
value: UninitializedModel,
Expand Down Expand Up @@ -80,6 +88,7 @@ type ErroredChunk<T> = {
};
type SomeChunk<T> =
| PendingChunk<T>
| BlockedChunk<T>
| ResolvedModelChunk<T>
| ResolvedModuleChunk<T>
| InitializedChunk<T>
Expand Down Expand Up @@ -115,6 +124,7 @@ Chunk.prototype.then = function<T>(
resolve(chunk.value);
break;
case PENDING:
case BLOCKED:
if (resolve) {
if (chunk.value === null) {
chunk.value = [];
Expand Down Expand Up @@ -159,8 +169,9 @@ function readChunk<T>(chunk: SomeChunk<T>): T {
case INITIALIZED:
return chunk.value;
case PENDING:
case BLOCKED:
// eslint-disable-next-line no-throw-literal
throw (chunk: Thenable<T>);
throw ((chunk: any): Thenable<T>);
default:
throw chunk.reason;
}
Expand All @@ -177,6 +188,11 @@ function createPendingChunk<T>(response: Response): PendingChunk<T> {
return new Chunk(PENDING, null, null, response);
}

function createBlockedChunk<T>(response: Response): BlockedChunk<T> {
// $FlowFixMe Flow doesn't support functions as constructors
return new Chunk(BLOCKED, null, null, response);
}

function createErrorChunk<T>(
response: Response,
error: Error,
Expand Down Expand Up @@ -210,6 +226,7 @@ function wakeChunkIfInitialized<T>(
wakeChunk(resolveListeners, chunk.value);
break;
case PENDING:
case BLOCKED:
chunk.value = resolveListeners;
chunk.reason = rejectListeners;
break;
Expand All @@ -222,7 +239,7 @@ function wakeChunkIfInitialized<T>(
}

function triggerErrorOnChunk<T>(chunk: SomeChunk<T>, error: mixed): void {
if (chunk.status !== PENDING) {
if (chunk.status !== PENDING && chunk.status !== BLOCKED) {
// We already resolved. We didn't expect to see this.
return;
}
Expand Down Expand Up @@ -278,7 +295,7 @@ function resolveModuleChunk<T>(
chunk: SomeChunk<T>,
value: ModuleReference<T>,
): void {
if (chunk.status !== PENDING) {
if (chunk.status !== PENDING && chunk.status !== BLOCKED) {
// We already resolved. We didn't expect to see this.
return;
}
Expand Down Expand Up @@ -308,11 +325,11 @@ function initializeModelChunk<T>(chunk: ResolvedModelChunk<T>): void {
) {
initializingChunkBlockedModel.value = value;
// We discovered new dependencies on modules that are not yet resolved.
// We have to return to the PENDING state until they're resolved.
const pendingChunk: PendingChunk<T> = (chunk: any);
pendingChunk.status = PENDING;
pendingChunk.value = null;
pendingChunk.reason = null;
// We have to go the BLOCKED state until they're resolved.
const blockedChunk: BlockedChunk<T> = (chunk: any);
blockedChunk.status = BLOCKED;
blockedChunk.value = null;
blockedChunk.reason = null;
} else {
const initializedChunk: InitializedChunk<T> = (chunk: any);
initializedChunk.status = INITIALIZED;
Expand Down Expand Up @@ -348,7 +365,9 @@ export function reportGlobalError(response: Response, error: Error): void {
// If this chunk was already resolved or errored, it won't
// trigger an error but if it wasn't then we need to
// because we won't be getting any new data to resolve it.
triggerErrorOnChunk(chunk, error);
if (chunk.status === PENDING) {
triggerErrorOnChunk(chunk, error);
}
});
}

Expand Down Expand Up @@ -433,7 +452,7 @@ function createModelResolver<T>(
parentObject[key] = value;
blocked.deps--;
if (blocked.deps === 0) {
if (chunk.status !== PENDING) {
if (chunk.status !== BLOCKED) {
return;
}
const resolveListeners = chunk.value;
Expand Down Expand Up @@ -480,6 +499,7 @@ export function parseModelString(
case INITIALIZED:
return chunk.value;
case PENDING:
case BLOCKED:
const parentChunk = initializingChunk;
chunk.then(
createModelResolver(parentChunk, parentObject, key),
Expand Down Expand Up @@ -573,21 +593,28 @@ export function resolveModule(
// that we'll need them.
const promise = preloadModule(moduleReference);
if (promise) {
let pendingChunk;
let blockedChunk: BlockedChunk<any>;
if (!chunk) {
pendingChunk = createPendingChunk(response);
chunks.set(id, pendingChunk);
// Technically, we should just treat promise as the chunk in this
// case. Because it'll just behave as any other promise.
blockedChunk = createBlockedChunk(response);
chunks.set(id, blockedChunk);
} else {
pendingChunk = chunk;
// This can't actually happen because we don't have any forward
// references to modules.
blockedChunk = (chunk: any);
blockedChunk.status = BLOCKED;
}
promise.then(
() => resolveModuleChunk(pendingChunk, moduleReference),
error => triggerErrorOnChunk(pendingChunk, error),
() => resolveModuleChunk(blockedChunk, moduleReference),
error => triggerErrorOnChunk(blockedChunk, error),
);
} else {
if (!chunk) {
chunks.set(id, createResolvedModuleChunk(response, moduleReference));
} else {
// This can't actually happen because we don't have any forward
// references to modules.
resolveModuleChunk(chunk, moduleReference);
}
}
Expand Down
13 changes: 6 additions & 7 deletions packages/react-reconciler/src/ReactFiberWakeable.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,6 @@ export function trackSuspendedWakeable(wakeable: Wakeable) {
// If the thenable doesn't have a status, set it to "pending" and attach
// a listener that will update its status and result when it resolves.
switch (thenable.status) {
case 'pending':
// Since the status is already "pending", we can assume it will be updated
// when it resolves, either by React or something in userspace.
break;
case 'fulfilled':
case 'rejected':
// A thenable that already resolved shouldn't have been thrown, so this is
Expand All @@ -75,9 +71,12 @@ export function trackSuspendedWakeable(wakeable: Wakeable) {
suspendedThenable = null;
break;
default: {
// TODO: Only instrument the thenable if the status if not defined. If
// it's defined, but an unknown value, assume it's been instrumented by
// some custom userspace implementation.
if (typeof thenable.status === 'string') {
// Only instrument the thenable if the status if not defined. If
// it's defined, but an unknown value, assume it's been instrumented by
// some custom userspace implementation. We treat it as "pending".
break;
}
const pendingThenable: PendingThenable<mixed> = (thenable: any);
pendingThenable.status = 'pending';
pendingThenable.then(
Expand Down
13 changes: 6 additions & 7 deletions packages/react-reconciler/src/ReactFiberWakeable.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,6 @@ export function trackSuspendedWakeable(wakeable: Wakeable) {
// If the thenable doesn't have a status, set it to "pending" and attach
// a listener that will update its status and result when it resolves.
switch (thenable.status) {
case 'pending':
// Since the status is already "pending", we can assume it will be updated
// when it resolves, either by React or something in userspace.
break;
case 'fulfilled':
case 'rejected':
// A thenable that already resolved shouldn't have been thrown, so this is
Expand All @@ -75,9 +71,12 @@ export function trackSuspendedWakeable(wakeable: Wakeable) {
suspendedThenable = null;
break;
default: {
// TODO: Only instrument the thenable if the status if not defined. If
// it's defined, but an unknown value, assume it's been instrumented by
// some custom userspace implementation.
if (typeof thenable.status === 'string') {
// Only instrument the thenable if the status if not defined. If
// it's defined, but an unknown value, assume it's been instrumented by
// some custom userspace implementation. We treat it as "pending".
break;
}
const pendingThenable: PendingThenable<mixed> = (thenable: any);
pendingThenable.status = 'pending';
pendingThenable.then(
Expand Down
13 changes: 6 additions & 7 deletions packages/react-server/src/ReactFizzWakeable.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,6 @@ export function trackSuspendedWakeable(wakeable: Wakeable) {
// If the thenable doesn't have a status, set it to "pending" and attach
// a listener that will update its status and result when it resolves.
switch (thenable.status) {
case 'pending':
// Since the status is already "pending", we can assume it will be updated
// when it resolves, either by React or something in userspace.
break;
case 'fulfilled':
case 'rejected':
// A thenable that already resolved shouldn't have been thrown, so this is
Expand All @@ -57,9 +53,12 @@ export function trackSuspendedWakeable(wakeable: Wakeable) {
// TODO: Log a warning?
break;
default: {
// TODO: Only instrument the thenable if the status if not defined. If
// it's defined, but an unknown value, assume it's been instrumented by
// some custom userspace implementation.
if (typeof thenable.status === 'string') {
// Only instrument the thenable if the status if not defined. If
// it's defined, but an unknown value, assume it's been instrumented by
// some custom userspace implementation. We treat it as "pending".
break;
}
const pendingThenable: PendingThenable<mixed> = (thenable: any);
pendingThenable.status = 'pending';
pendingThenable.then(
Expand Down
13 changes: 6 additions & 7 deletions packages/react-server/src/ReactFlightWakeable.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,6 @@ export function trackSuspendedWakeable(wakeable: Wakeable) {
// If the thenable doesn't have a status, set it to "pending" and attach
// a listener that will update its status and result when it resolves.
switch (thenable.status) {
case 'pending':
// Since the status is already "pending", we can assume it will be updated
// when it resolves, either by React or something in userspace.
break;
case 'fulfilled':
case 'rejected':
// A thenable that already resolved shouldn't have been thrown, so this is
Expand All @@ -57,9 +53,12 @@ export function trackSuspendedWakeable(wakeable: Wakeable) {
// TODO: Log a warning?
break;
default: {
// TODO: Only instrument the thenable if the status if not defined. If
// it's defined, but an unknown value, assume it's been instrumented by
// some custom userspace implementation.
if (typeof thenable.status === 'string') {
// Only instrument the thenable if the status if not defined. If
// it's defined, but an unknown value, assume it's been instrumented by
// some custom userspace implementation. We treat it as "pending".
break;
}
const pendingThenable: PendingThenable<mixed> = (thenable: any);
pendingThenable.status = 'pending';
pendingThenable.then(
Expand Down

0 comments on commit 1c481e9

Please sign in to comment.