Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fizz] Support abort reasons #24680

Merged
merged 6 commits into from Jun 8, 2022
Merged

[Fizz] Support abort reasons #24680

merged 6 commits into from Jun 8, 2022

Conversation

gnoff
Copy link
Collaborator

@gnoff gnoff commented Jun 6, 2022

Fizz supports aborting the render but does not currently accept a reason. The various render functions that use Fizz have some automatic and some user-controlled abort semantics that can be useful to communicate with the running program and users about why an Abort happened.

This change implements abort reasons for renderToReadableStream and renderToPipeable stream as well as legacy renderers such as renderToString and related implementations.

For AbortController implementations the reason passed to the abort method is forwarded to Fizz and sent to the onError handler. If no reason is provided the AbortController should construct an AbortError DOMException and as a fallback Fizz will generate a similar error in the absence of a reason

For pipeable streams, an abort function is returned alongside pipe which already accepted a reason. That reason is now forwarded to Fizz and the implementation described above.

For legacy renderers there is no exposed abort functionality but it is used internally and the reasons provided give useful context to, for instance to the fact that Suspense is not supported in renderToString-like renderers

Some notable special case reasons are included below
If no reason is provided then a manufactured reason "signal is aborted without reason"
If a writable stream errors then a reason "The destination stream errored while writing data."
If a writable stream closes early then a reason "The destination stream closed early."

@sizebot
Copy link

@sizebot sizebot commented Jun 6, 2022

Comparing: 7e8a020...cdeeb04

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 131.83 kB 131.83 kB = 42.33 kB 42.33 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 137.10 kB 137.10 kB = 43.92 kB 43.92 kB
facebook-www/ReactDOM-prod.classic.js = 441.04 kB 441.04 kB = 80.69 kB 80.69 kB
facebook-www/ReactDOM-prod.modern.js = 426.35 kB 426.35 kB = 78.49 kB 78.49 kB
facebook-www/ReactDOMForked-prod.classic.js = 440.57 kB 440.57 kB = 80.61 kB 80.61 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.browser.production.min.js +1.84% 34.39 kB 35.02 kB +1.63% 11.43 kB 11.61 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.browser.production.min.js +1.84% 34.41 kB 35.04 kB +1.62% 11.45 kB 11.63 kB
oss-stable-semver/react-dom/umd/react-dom-server-legacy.browser.production.min.js +1.84% 34.49 kB 35.13 kB +1.51% 11.55 kB 11.73 kB
oss-stable/react-dom/umd/react-dom-server-legacy.browser.production.min.js +1.84% 34.52 kB 35.15 kB +1.51% 11.57 kB 11.75 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.browser.production.min.js +1.82% 34.83 kB 35.46 kB +1.57% 11.59 kB 11.77 kB
oss-experimental/react-dom/umd/react-dom-server-legacy.browser.production.min.js +1.81% 34.94 kB 35.57 kB +1.48% 11.71 kB 11.89 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.node.production.min.js +1.66% 38.16 kB 38.80 kB +1.21% 12.61 kB 12.77 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.node.production.min.js +1.66% 38.19 kB 38.82 kB +1.20% 12.63 kB 12.79 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.node.production.min.js +1.64% 38.66 kB 39.30 kB +1.20% 12.80 kB 12.95 kB
facebook-www/ReactDOMServer-prod.modern.js +1.14% 79.32 kB 80.22 kB +1.33% 16.51 kB 16.73 kB
facebook-www/ReactDOMServer-dev.modern.js +0.55% 248.43 kB 249.79 kB +0.58% 58.99 kB 59.33 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.browser.development.js +0.54% 242.60 kB 243.90 kB +0.50% 58.50 kB 58.79 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.browser.development.js +0.54% 242.62 kB 243.92 kB +0.50% 58.51 kB 58.81 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.browser.development.js +0.53% 244.17 kB 245.47 kB +0.56% 58.91 kB 59.24 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.node.development.js +0.53% 244.31 kB 245.61 kB +0.52% 58.99 kB 59.30 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.node.development.js +0.53% 244.33 kB 245.63 kB +0.50% 59.02 kB 59.31 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.node.development.js +0.53% 245.88 kB 247.18 kB +0.46% 59.43 kB 59.70 kB
oss-stable-semver/react-server/cjs/react-server.development.js +0.52% 129.56 kB 130.24 kB +0.51% 32.41 kB 32.58 kB
oss-stable/react-server/cjs/react-server.development.js +0.52% 129.56 kB 130.24 kB +0.51% 32.41 kB 32.58 kB
oss-stable-semver/react-dom/umd/react-dom-server-legacy.browser.development.js +0.52% 254.46 kB 255.79 kB +0.55% 59.16 kB 59.49 kB
oss-stable/react-dom/umd/react-dom-server-legacy.browser.development.js +0.52% 254.49 kB 255.82 kB +0.54% 59.18 kB 59.51 kB
oss-experimental/react-server/cjs/react-server.development.js +0.52% 130.43 kB 131.11 kB +0.49% 32.62 kB 32.78 kB
oss-experimental/react-dom/umd/react-dom-server-legacy.browser.development.js +0.52% 256.12 kB 257.45 kB +0.51% 59.61 kB 59.91 kB
oss-stable-semver/react-dom/cjs/react-dom-server.node.production.min.js +0.45% 38.88 kB 39.06 kB +0.39% 13.24 kB 13.29 kB
oss-stable/react-dom/cjs/react-dom-server.node.production.min.js +0.45% 38.91 kB 39.08 kB +0.37% 13.26 kB 13.31 kB
oss-experimental/react-dom/cjs/react-dom-server.node.production.min.js +0.44% 39.38 kB 39.56 kB +0.37% 13.42 kB 13.47 kB
oss-stable-semver/react-dom/cjs/react-dom-server.node.development.js +0.42% 243.15 kB 244.17 kB +0.36% 58.62 kB 58.83 kB
oss-stable/react-dom/cjs/react-dom-server.node.development.js +0.42% 243.18 kB 244.19 kB +0.37% 58.64 kB 58.86 kB
oss-experimental/react-dom/cjs/react-dom-server.node.development.js +0.42% 244.72 kB 245.74 kB +0.36% 59.07 kB 59.28 kB
facebook-www/ReactDOMServerStreaming-prod.modern.js +0.34% 82.94 kB 83.23 kB +0.10% 17.52 kB 17.54 kB
oss-stable-semver/react-dom/cjs/react-dom-server.browser.development.js +0.29% 242.10 kB 242.79 kB +0.30% 58.74 kB 58.91 kB
oss-stable/react-dom/cjs/react-dom-server.browser.development.js +0.29% 242.12 kB 242.81 kB +0.30% 58.76 kB 58.93 kB
oss-stable-semver/react-dom/umd/react-dom-server.browser.development.js +0.29% 253.93 kB 254.65 kB +0.29% 59.42 kB 59.60 kB
oss-stable/react-dom/umd/react-dom-server.browser.development.js +0.29% 253.95 kB 254.68 kB +0.29% 59.45 kB 59.62 kB
oss-experimental/react-dom/cjs/react-dom-server.browser.development.js +0.28% 243.66 kB 244.36 kB +0.26% 59.19 kB 59.35 kB
oss-experimental/react-dom/umd/react-dom-server.browser.development.js +0.28% 255.59 kB 256.31 kB +0.29% 59.85 kB 60.02 kB
facebook-www/ReactDOMServerStreaming-dev.modern.js +0.28% 244.71 kB 245.40 kB +0.27% 58.14 kB 58.30 kB

Generated by 🚫 dangerJS against cdeeb04

@sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Jun 6, 2022

This looks like it affected the file size of oss-stable/react-server/cjs/react-server.production.min.js.

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

AbortController allows a reason to be passed to abort(). We should probably pass that object through and let that be the object that's passed to onError. If it's a string or something with .message, we use that as the error message on the client.

Then we can pass a custom error from renderToString to the abort function.

So FizzServer exports abort(reason) and we call that with signal.reason in the Web Streams form. Node streams abort() can start accepting a reason too.

@gnoff
Copy link
Collaborator Author

@gnoff gnoff commented Jun 7, 2022

I like that a lot better but there is one thing that bothers me

It feels wrong for the reason to be so prescriptive about where it will show up. It doesn't make sense for instance for the renderToString function to pass along as a reason "This Suspense boundary did not finish rendering" however that is the correct wording for the error we eventually want to show up in the client.

I solved this by having Fizz append the reason to the error message which is marginally better. But now that I've exposed the ability to send custom reasons in, if someone passes in an Error, i'd have to either give up providing any error guidance in abortTask and just pass the error through, or manipulate the error-like object by reading the message property and reconstructing the error which might hide stack details the implementor would find useful.

Additionally, the current implementation restricts reasons to strings or error-like objects. But onError takes mixed and I suppose one should really be allowed to pass in anything for the reason. But this just exacerbates the challenge of what to do about the messaging for each boundary

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

There's some discussion here. My understanding is that the current WHATWG recommendation is to pass the reason straight through and throw whatever it was.

However, Node.js chose not to do that for their APIs and instead attach it to the .cause of the error.

whatwg/dom#1033 (comment)

I think the way to think about it is that the client gets a synthetic wrapper error and really the "cause" is the reason on the client. That's the part that's associated with a suspense boundary and why that message matters there is all on the client.

onError doesn't have anything to do with that it's a suspense boundary that didn't finish. That's just that some error happened.

So I think the move here is that onError accepts the raw reason while the serialized error is a synthetic wrapper.

Conceptually it's like:

class SuspenseBoundaryError extends Error {
  constructor(cause) {
    this.message = `The server did not finish this Suspense boundary: ${cause.message}`;
    this.cause = cause;
  }
}

function report(reason) {
  onError(reason);
  reportToClient(new SuspenseBoundaryError(reason));
}

However since we lose the actual instances and classes as part of the serialization anyway. The cause object isn't as important, it's also a fairly new feature anyway.

So I feel like it's fine that we just append something like The server did not finish this Suspense boundary to the message of the reason - but only in the part that gets serialized to the client. It's also just DEV only.

// This is called to early terminate a request. It puts all pending boundaries in client rendered state.
export function abort(request: Request): void {
export function abort(request: Request, reason: ?AbortReason): void {
request.abortReason = reason;
try {
const abortableTasks = request.abortableTasks;
abortableTasks.forEach(abortTask, request);
Copy link
Collaborator

@sebmarkbage sebmarkbage Jun 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of stashing it on the request and adding another field, you can just pass it through here (and the same thing inside abortTask). You use a closure instead of the overly clever context argument.

Copy link
Collaborator Author

@gnoff gnoff Jun 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that was my first instinct. Sometimes I assume there is some 4d chess going on and the avoidance of the closure is paramount but once you point it out it's obvious that it isn't that important. 🤪

Copy link
Collaborator

@sebmarkbage sebmarkbage Jun 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In hot paths that's the right instinct but in this case it's an exceptional case.

@@ -2158,8 +2175,11 @@ export function startFlowing(request: Request, destination: Destination): void {
}
}

type AbortReason = string | {message: string, ...};
Copy link
Collaborator

@sebmarkbage sebmarkbage Jun 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little unclear what the types of these should be but my understanding is that it's moving towards being effectively a throw given these apis:

https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/throwIfAborted

As well as similar recommendations to throw the signal.reason. That's not always how built-ins deal with them though - e.g. fetch.

Throws can be anything really (but should typically be Error objects).

Type wise they're mixed though which is why onError accepts mixed. This needs to also accept mixed. So you don't really need this type.

packages/react-server/src/ReactFizzServer.js Show resolved Hide resolved
@sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Jun 7, 2022

In the standard AbortSignal I think you get an AbortError with the message signal is aborted without reason by default if you don't provide a reason. We should do the same for the Node.js API or the Web API if signal.reason === undefined (e.g. if it's not implemented).

Maybe this part can probably be implemented in ReactFizzServer if the reason is undefined.

@@ -31,10 +31,11 @@ function onError() {
// Non-fatal errors are ignored.
}

function renderToStringImpl(
export function renderToStringImpl(
Copy link
Collaborator

@sebmarkbage sebmarkbage Jun 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is a build entry point:

https://github.com/facebook/react/blob/main/scripts/rollup/bundles.js#L273

Exporting something here exposes it to UMD builds as well as the CJS builds.

If you want to share an internal helper you have to move this function to a third module that is imported by both.

Fizz supports aborting the render but does not currently accept a reason. The various render functions that use Fizz have some automatic and some user-controlled abort semantics that can be useful to communicate with the running program and users about why an Abort happened.

This change implements abort reasons for renderToReadableStream and renderToPipeable stream as well as legacy renderers such as renderToString and related implementations.

For AbortController implementations the reason passed to the abort method is forwarded to Fizz and sent to the onError handler. If no reason is provided the AbortController should construct an AbortError DOMException and as a fallback Fizz will generate a similar error in the absense of a reason

For pipeable  streams, an abort function is returned alongside pipe which already accepted a reason. That reason is now forwarded to Fizz and the implementation described above.

For legacy renderers there is no exposed abort functionality but it is used internally and the reasons provided give useful context to, for instance to the fact that Suspsense is not supported in renderToString-like renderers

Some notable special case reasons are included below
If no reason is provided then a manufactured reason "signal is aborted without reason"
If a writable stream errors then a reason "The destination stream errored while writing data."
If a writable stream closes early then a reason "The destination stream closed early."
@gnoff gnoff force-pushed the abort-error-message branch from bd66c7b to 20331f1 Compare Jun 7, 2022
@gnoff gnoff changed the title [Fizz] Support more nuanced errors for aborts when rending using legacy methods [Fizz] Support abort reasons Jun 7, 2022
@gnoff gnoff force-pushed the abort-error-message branch from b952b61 to 0438912 Compare Jun 7, 2022
'error',
createAbortHandler(
request,
// eslint-disable-next-line react-internal/prod-error-codes
Copy link
Collaborator

@sebmarkbage sebmarkbage Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason this should not be minified?

Copy link
Collaborator Author

@gnoff gnoff Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll be honest, I’m not sure why any errors in fizz should be minified. The code stays in the server so payload is not the most critical thing and it forks the dev/prod behavior of onError which would likely lead to many confused devs not sure why their logic is broken in prod. And we don’t serialize the error and send to the client.

Is there a benefit to minifying that I’m missing?

Copy link
Collaborator

@sebmarkbage sebmarkbage Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the server builds (in particular the Node ones) don't minify but they do it globally for the whole build in bundles.js.

The browser ones do minify, partially because they're actually sometimes used in browsers. In particular renderToString is commonly used (although probably shouldn't) but you can also use streams e.g. in Service Workers.

That said, we should probably be size conscious even for the server because cold-starts on the server is such a significant issue.

So it should probably be up to those settings rather than on a per-error level.

Copy link
Collaborator Author

@gnoff gnoff Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what about the runtime divergence? How does one even test that their apps onError logic does what it is supposed to? Some errors are hard to trigger on purpose. I suppose this’ll be a challenge for onRecoverableError in the client. Perhaps a eager-lazy loading of the real error messages could be implemented if and only if you are trying to do logic with them?

Copy link
Collaborator

@sebmarkbage sebmarkbage Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For logging, it's not really different than client-side logging is today. You log the minified error and then you can automatically reverse it from the codes.json in UI (like Sentry does I believe).

I imagine you typically would log them both to the same kind of system anyway.

Copy link
Collaborator

@sebmarkbage sebmarkbage Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For things like branching on semantic logic like "if this was intentionally aborted, do something different" you really shouldn't use the error message anyway. E.g. built-in errors can be translated or change at the browser's will. There's things like .name or instanceof for that.

Copy link
Collaborator Author

@gnoff gnoff Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As always, you're right :)

let error =
reason === undefined
? // eslint-disable-next-line react-internal/prod-error-codes
new Error('signal is aborted without reason')
Copy link
Collaborator

@sebmarkbage sebmarkbage Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, on a second thought. This is kind of a weird error message in the Node API because there was no signal involved.

@@ -419,5 +419,6 @@
"431": "React elements are not allowed in ServerContext",
"432": "This Suspense boundary was aborted by the server.",
"433": "useId can only be used while React is rendering",
"434": "`dangerouslySetInnerHTML` does not make sense on <title>."
"434": "`dangerouslySetInnerHTML` does not make sense on <title>.",
"435": "The server did not finish this Suspense boundary. The server used \"%s\" which does not support Suspense. If you intended for this Suspense boundary to render the fallback content on the server consider throwing an Error somewhere within the Suspense boundary. If you intended to have the server wait for the suspended component please switch to \"%s\" which supports Suspense on the server"
Copy link
Collaborator

@sebmarkbage sebmarkbage Jun 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this error anywhere now since these are just passed as strings and not new Error(...) they don't minified.

@gnoff gnoff merged commit b345523 into facebook:main Jun 8, 2022
36 checks passed
@gnoff gnoff deleted the abort-error-message branch Jun 8, 2022
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Jun 15, 2022
Summary:
This sync includes the following changes:
- **[5cc2487e0](facebook/react@5cc2487e0 )**: bump versions for next release ([#24725](facebook/react#24725)) //<Josh Story>//
- **[54f17e490](facebook/react@54f17e490 )**: [Transition Tracing] Fix Cache and Transitions Pop Order ([#24719](facebook/react#24719)) //<Luna Ruan>//
- **[7cf8dfd94](facebook/react@7cf8dfd94 )**: [Transition Tracing] Create/Process Marker Complete Callback ([#24700](facebook/react#24700)) //<Luna Ruan>//
- **[327e4a1f9](facebook/react@327e4a1f9 )**: [Follow-up] Land enableClientRenderFallbackOnTextMismatch //<Andrew Clark>//
- **[a8c9cb18b](facebook/react@a8c9cb18b )**: Land enableSuspenseLayoutEffectSemantics flag ([#24713](facebook/react#24713)) //<Andrew Clark>//
- **[a8555c308](facebook/react@a8555c308 )**: [Transition Tracing] Add Tracing Marker Stack ([#24661](facebook/react#24661)) //<Luna Ruan>//
- **[8186b1937](facebook/react@8186b1937 )**: Check for infinite update loops even if unmounted ([#24697](facebook/react#24697)) //<Andrew Clark>//
- **[060505e9d](facebook/react@060505e9d )**: Fix misapplying prod error opt-out ([#24688](facebook/react#24688)) //<Josh Story>//
- **[47944142f](facebook/react@47944142f )**: `now` isn't part of the react-reconciler config anymore ([#24689](facebook/react#24689)) //<Mathieu Dutour>//
- **[b34552352](facebook/react@b34552352 )**: [Fizz] Support abort reasons ([#24680](facebook/react#24680)) //<Josh Story>//
- **[79f54c16d](facebook/react@79f54c16d )**: Bugfix: Revealing a hidden update ([#24685](facebook/react#24685)) //<Andrew Clark>//
- **[7e8a020a4](facebook/react@7e8a020a4 )**: Remove extra Server Context argument ([#24683](facebook/react#24683)) //<Sebastian Markbåge>//
- **[4f29ba1cc](facebook/react@4f29ba1cc )**: support errorInfo in onRecoverableError ([#24591](facebook/react#24591)) //<Josh Story>//
- **[1cd90d2cc](facebook/react@1cd90d2cc )**: Refactor of interleaved ("concurrent") update queue ([#24663](facebook/react#24663)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions d300ceb...256aefb

jest_e2e[run_all_tests]

Reviewed By: cortinico

Differential Revision: D37155957

fbshipit-source-id: 4c0afc95abe8fa13c3803584922c8dc0059ff562
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Aug 8, 2022
Summary:
Sync goes to v18.2 release.

I had to manually trigger CircleCI builds because TTL for build artefacts is 30 days.

This sync includes the following changes:
- **[060505e9d](facebook/react@060505e9d )**: Fix misapplying prod error opt-out ([#24688](facebook/react#24688)) //<Josh Story>//
- **[47944142f](facebook/react@47944142f )**: `now` isn't part of the react-reconciler config anymore ([#24689](facebook/react#24689)) //<Mathieu Dutour>//
- **[b34552352](facebook/react@b34552352 )**: [Fizz] Support abort reasons ([#24680](facebook/react#24680)) //<Josh Story>//
- **[79f54c16d](facebook/react@79f54c16d )**: Bugfix: Revealing a hidden update ([#24685](facebook/react#24685)) //<Andrew Clark>//
- **[7e8a020a4](facebook/react@7e8a020a4 )**: Remove extra Server Context argument ([#24683](facebook/react#24683)) //<Sebastian Markbåge>//
- **[4f29ba1cc](facebook/react@4f29ba1cc )**: support errorInfo in onRecoverableError ([#24591](facebook/react#24591)) //<Josh Story>//
- **[1cd90d2cc](facebook/react@1cd90d2cc )**: Refactor of interleaved ("concurrent") update queue ([#24663](facebook/react#24663)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions d300ceb...9e3b772

jest_e2e[run_all_tests]

Reviewed By: JoshuaGross

Differential Revision: D38496392

fbshipit-source-id: 3ecffc2b3354104562eb23a2643fe0a37a01a7e6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants