From 1c481e9873fb5460b2fa050c78ddb2ef49dd6585 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 13 Sep 2022 23:53:19 -0400 Subject: [PATCH] Treat blocked modules or models as a special status 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. --- .../react-client/src/ReactFlightClient.js | 59 ++++++++++++++----- .../src/ReactFiberWakeable.new.js | 13 ++-- .../src/ReactFiberWakeable.old.js | 13 ++-- .../react-server/src/ReactFizzWakeable.js | 13 ++-- .../react-server/src/ReactFlightWakeable.js | 13 ++-- 5 files changed, 67 insertions(+), 44 deletions(-) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index dc8cd2e0ce1ba..3ca33ff00e3d3 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -38,6 +38,7 @@ export type JSONValue = | $ReadOnlyArray; const PENDING = 'pending'; +const BLOCKED = 'blocked'; const RESOLVED_MODEL = 'resolved_model'; const RESOLVED_MODULE = 'resolved_module'; const INITIALIZED = 'fulfilled'; @@ -50,6 +51,13 @@ type PendingChunk = { _response: Response, then(resolve: (T) => mixed, reject: (mixed) => mixed): void, }; +type BlockedChunk = { + status: 'blocked', + value: null | Array<(T) => mixed>, + reason: null | Array<(mixed) => mixed>, + _response: Response, + then(resolve: (T) => mixed, reject: (mixed) => mixed): void, +}; type ResolvedModelChunk = { status: 'resolved_model', value: UninitializedModel, @@ -80,6 +88,7 @@ type ErroredChunk = { }; type SomeChunk = | PendingChunk + | BlockedChunk | ResolvedModelChunk | ResolvedModuleChunk | InitializedChunk @@ -115,6 +124,7 @@ Chunk.prototype.then = function( resolve(chunk.value); break; case PENDING: + case BLOCKED: if (resolve) { if (chunk.value === null) { chunk.value = []; @@ -159,8 +169,9 @@ function readChunk(chunk: SomeChunk): T { case INITIALIZED: return chunk.value; case PENDING: + case BLOCKED: // eslint-disable-next-line no-throw-literal - throw (chunk: Thenable); + throw ((chunk: any): Thenable); default: throw chunk.reason; } @@ -177,6 +188,11 @@ function createPendingChunk(response: Response): PendingChunk { return new Chunk(PENDING, null, null, response); } +function createBlockedChunk(response: Response): BlockedChunk { + // $FlowFixMe Flow doesn't support functions as constructors + return new Chunk(BLOCKED, null, null, response); +} + function createErrorChunk( response: Response, error: Error, @@ -210,6 +226,7 @@ function wakeChunkIfInitialized( wakeChunk(resolveListeners, chunk.value); break; case PENDING: + case BLOCKED: chunk.value = resolveListeners; chunk.reason = rejectListeners; break; @@ -222,7 +239,7 @@ function wakeChunkIfInitialized( } function triggerErrorOnChunk(chunk: SomeChunk, 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; } @@ -278,7 +295,7 @@ function resolveModuleChunk( chunk: SomeChunk, value: ModuleReference, ): void { - if (chunk.status !== PENDING) { + if (chunk.status !== PENDING && chunk.status !== BLOCKED) { // We already resolved. We didn't expect to see this. return; } @@ -308,11 +325,11 @@ function initializeModelChunk(chunk: ResolvedModelChunk): 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 = (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 = (chunk: any); + blockedChunk.status = BLOCKED; + blockedChunk.value = null; + blockedChunk.reason = null; } else { const initializedChunk: InitializedChunk = (chunk: any); initializedChunk.status = INITIALIZED; @@ -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); + } }); } @@ -433,7 +452,7 @@ function createModelResolver( parentObject[key] = value; blocked.deps--; if (blocked.deps === 0) { - if (chunk.status !== PENDING) { + if (chunk.status !== BLOCKED) { return; } const resolveListeners = chunk.value; @@ -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), @@ -573,21 +593,28 @@ export function resolveModule( // that we'll need them. const promise = preloadModule(moduleReference); if (promise) { - let pendingChunk; + let blockedChunk: BlockedChunk; 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); } } diff --git a/packages/react-reconciler/src/ReactFiberWakeable.new.js b/packages/react-reconciler/src/ReactFiberWakeable.new.js index 11f470cfab888..fad1d49f4474e 100644 --- a/packages/react-reconciler/src/ReactFiberWakeable.new.js +++ b/packages/react-reconciler/src/ReactFiberWakeable.new.js @@ -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 @@ -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 = (thenable: any); pendingThenable.status = 'pending'; pendingThenable.then( diff --git a/packages/react-reconciler/src/ReactFiberWakeable.old.js b/packages/react-reconciler/src/ReactFiberWakeable.old.js index 11f470cfab888..fad1d49f4474e 100644 --- a/packages/react-reconciler/src/ReactFiberWakeable.old.js +++ b/packages/react-reconciler/src/ReactFiberWakeable.old.js @@ -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 @@ -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 = (thenable: any); pendingThenable.status = 'pending'; pendingThenable.then( diff --git a/packages/react-server/src/ReactFizzWakeable.js b/packages/react-server/src/ReactFizzWakeable.js index 5a42ed396c4d9..aea0b507c8ca7 100644 --- a/packages/react-server/src/ReactFizzWakeable.js +++ b/packages/react-server/src/ReactFizzWakeable.js @@ -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 @@ -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 = (thenable: any); pendingThenable.status = 'pending'; pendingThenable.then( diff --git a/packages/react-server/src/ReactFlightWakeable.js b/packages/react-server/src/ReactFlightWakeable.js index c3eda3c16aee6..b1ff2aa5eb3ef 100644 --- a/packages/react-server/src/ReactFlightWakeable.js +++ b/packages/react-server/src/ReactFlightWakeable.js @@ -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 @@ -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 = (thenable: any); pendingThenable.status = 'pending'; pendingThenable.then(