[Fiber] New error boundary semantics #8304

acdlite · 2016-11-16T16:59:56Z

Error boundaries, take 4 :) Supersedes #8227.

Recovering from an error boundary no longer uses Task priority by
default. The work is scheduled using whatever priority created the
error.
Error handling is now a side-effect that happens during the
commit phase.
The default behavior of an error boundary is to render null. Errors
do not propagate except when an boundary fails. Conceptually, this would
be like throwing an error from a catch block.
A host container is treated like a no-op error boundary. The first
error captured by a host container is thrown at the end of the batch.
Like with normal error boundaries, the entire tree is unmounted.

TODO:

Rebase on latest master
Fix last failing test
Ensure the context stack correctly unwinds on errors

The scheduler is getting a bit messy / difficult to work with, especially with all the perform- functions. I'll try to clean some of the accumulated cruft in a future PR.

acdlite · 2016-11-17T00:30:03Z

Ready for review.

I may try to remove the need to unwind the context stack by moving the memoizedProps assignment from beginWork to completeWork. Without that change, if you reset to the root as we discussed (#8272 (comment)), the parent fibers re-render because they never received memoizedProps.

Don't think that should block this PR from being merged, though.

sebmarkbage · 2016-11-17T13:18:29Z

src/renderers/shared/fiber/ReactFiberScheduler.js

+      }
+      if (primaryEffectTag & Err) {
+        primaryEffectTag ^= Err;
+      }


const NotCallbackOrError = ~(Callback | Err); let primaryEffectTag = effectfulFiber.effectTag & NotCallbackOrError;

gaearon · 2016-11-17T16:40:36Z

src/renderers/shared/stack/reconciler/__tests__/ReactComponent-test.js

@@ -332,7 +332,7 @@ describe('ReactComponent', () => {
    expect(callback.mock.calls.length).toBe(3);
  });

-  it('throws usefully when rendering badly-typed elements', () => {
+  xit('throws usefully when rendering badly-typed elements', () => {


Unintentional?

gaearon

Need to fix the hanging test & the test that fails with "cannot commit tree" that didn't fail with this message before.

acdlite · 2016-11-17T17:19:48Z

Fixed the hanging test; still working on the "cannot commit tree" one

outdated

gaearon · 2016-11-18T14:15:11Z

src/renderers/shared/fiber/ReactFiberScheduler.js

-          commitLifeCycles(current, effectfulFiber);
-        } finally {
-          priorityContext = previousPriorityContext;
-        }


Removing this try/finally block seems responsible for some of the test failures.
If I add it back I get:

--- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -470,7 +470,6 @@ src/renderers/dom/__tests__/ReactDOMProduction-test.js * should use prod React * should handle a simple flow * should call lifecycle methods -* should throw with an error code in production src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js * should render strings as children @@ -1021,8 +1020,12 @@ src/renderers/shared/shared/event/eventPlugins/__tests__/ResponderEventPlugin-te src/renderers/shared/stack/reconciler/__tests__/ReactComponent-test.js * should throw when supplying a ref outside of render method -* should support refs on owned components +* should warn when children are mutated during update * should not have refs on unmounted components +* should support new-style refs +* should support new-style refs with mixed-up owners +* should call refs at the correct time +* fires the callback after a component is rendered src/renderers/shared/stack/reconciler/__tests__/ReactComponentLifeCycle-test.js * should not reuse an instance when it has been unmounted @@ -1185,8 +1188,9 @@ src/renderers/shared/stack/reconciler/__tests__/ReactStatelessComponent-test.js * should update stateless component * should unmount stateless component * should pass context thru stateless component -* should provide a null ref +* should use correct name in key warning * should support default props and prop types +* should receive context * should work with arrow functions * should allow simple functions to return null * should allow simple functions to return false

gaearon · 2016-11-18T14:23:21Z

src/renderers/shared/fiber/ReactFiberContext.js

  return (
    fiber.tag === ClassComponent &&
+    // Instance might be null, if the fiber errored during construction
+    fiber.stateNode &&


Is this only true during unwinding? Can it be null in any other place in the code? We rely on stateNode existing on classes in a lot of places so I'm just trying to understand if something else could fail now.

gaearon · 2016-11-18T14:36:17Z

src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js

@@ -54,8 +54,8 @@ describe('ReactIncrementalErrorHandling', () => {

  it('propagates an error from a noop error boundary', () => {
    class NoopBoundary extends React.Component {


This doesn't match behavior of NoopBoundary in other test file. Can we either rename this one to RethrowBoundary, or even rename both and change the other file's tests for rethrowing? That's what they were testing previously.

gaearon · 2016-11-18T14:37:49Z

src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js

-    expect(container.firstChild.textContent).toBe('Caught an error: Hello.');
+    expect(container.firstChild.textContent).toBe(
+      ReactDOMFeatureFlags.useFiber ? '' : 'Caught an error: Hello.'
+    );


If you change NoopBoundary in this file to rethrow (and maybe rename it) then you won't need this condition. I would prefer that we have separate tests for "noop" and "rethrow" behavior now that they're different. It's fine to change most cases to test "rethrow" behavior (since that's what they were testing originally), and add one or two new tests for noop behavior (copy & paste + replace would be fine).

gaearon

Accepting on the condition that you fix newly failing tests and especially their flakiness (that they pass individually but fail together) and ensure rethrow behavior is still being tested in ErrorBoundaries-test.

gaearon

I'm really worried about these Cannot commit the same tree as before. errors. I'd prefer we don't merge this before they are addressed. I'm worried they'll slow us down because they make trees inconsistent between tests, and a failure in one test starts to affect other tests in weird ways. This wasn't the case in the previous version.

sebmarkbage · 2016-11-22T19:38:40Z

src/renderers/shared/fiber/ReactFiberScheduler.js

@@ -355,7 +385,15 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
      ReactFiberInstrumentation.debugTool.onWillBeginWork(workInProgress);
    }
    // See if beginning this work spawns more work.
-    let next = beginWork(current, workInProgress, nextPriorityLevel);
+    let next;
+    const isFailedWork = capturedErrors && capturedErrors.has(workInProgress);


Can we fork performUnitOfWork into a separate function then call it synchronous in a while loop while there is still capturedErrors? That way we don't have to check this condition thousands of times per frame (and whatever else optimizations can be done as a result).

I thought we made the decision that recovering from error work should use whatever priority created the error in the first place?

Hmm I think I may see how this would work, I'll try it out

So this is part of the follow up?

Yeah I'm working on it now

This avoids the need to create an export for every combination of bits.

We'll consolidate all these blocks in a future PR that refactors the commit phase to be separate from the perform work loop.

These tests are currently failing: ✕ catches render error in a boundary during partial deferred mounting ✕ catches render error in a boundary during animation mounting ✕ propagates an error from a noop error boundary during full deferred mounting ✕ propagates an error from a noop error boundary during partial deferred mounting ✕ propagates an error from a noop error boundary during animation mounting The observed behavior is that unstable_handleError() unexpected gets called twice: "ErrorBoundary render success", "BrokenRender", "ErrorBoundary unstable_handleError", + "ErrorBoundary render success", + "BrokenRender", + "ErrorBoundary unstable_handleError", "ErrorBoundary render error"

It was fixed in facebook#8451.

This is important so that the test at the end of performAndHandleErrors() knows it's safe to rethrow.

An error thrown from a host container should be "captured" by the host container itself

sebmarkbage · 2016-11-30T19:15:48Z

src/renderers/shared/fiber/ReactFiberScheduler.js

-          const current = effectfulFiber.alternate;
-          commitWork(current, effectfulFiber);
-          break;
+      try {


These try blocks should be outside the while loop. That way we can hoist them out of this function and avoid going in and out of an deopt path for each iteration.

Is my latest commit okay? Had to add another loop outside so that we can continue after an error

gaearon

I'm good with this

sebmarkbage · 2016-11-30T23:09:16Z

src/renderers/shared/fiber/ReactFiberScheduler.js

+    if (failedWork) {
+      // Host containers are a special case. If the failed work itself is a host
+      // container, then it acts as its own boundary. In all other cases, we
+      // ignore the work itself and only search through the parents.


When does this happen? If a callback to render throws?

ReactDOM.render(null, el);

If so, I'm not sure if we need to bother with this special case much since it happens at the end after all other work so there's no interleaving issue. Maybe for batching across roots but I'm not sure if this solution is sufficient in that case yet anyway.

It was added to fix the multiple roots issue.

sebmarkbage

ok, sorry for the delay. This should be good to go and we'll follow up with the other fix later.

acdlite · 2016-12-01T00:54:24Z

Travis takes forever. Merging anyway.

* Exit early in scheduleUpdate if a node's priority matches This is a performance optimization and is unobservable. However, it helps protect against regressions on the following invariants on which it relies: - The priority of a fiber is greater than or equal to the priority of all its descendent fibers. - If a tree has pending work priority, its root is scheduled. * New error boundary semantics - Recovering from an error boundary no longer uses Task priority by default. The work is scheduled using whatever priority created the error. - Error handling is now a side-effect that happens during the commit phase. - The default behavior of an error boundary is to render null. Errors do not propagate except when an boundary fails. Conceptually, this would be like throwing an error from a catch block. - A host container is treated like a no-op error boundary. The first error captured by a host container is thrown at the end of the batch. Like with normal error boundaries, the entire tree is unmounted. * Fix broken setState callback test * Add test for "unwinding" context when an error interrupts rendering * Switch over primary effect types only This avoids the need to create an export for every combination of bits. * Only continue the work loop if the error was successfully captured * Add more tests for incremental error handling These tests are currently failing: ✕ catches render error in a boundary during partial deferred mounting ✕ catches render error in a boundary during animation mounting ✕ propagates an error from a noop error boundary during full deferred mounting ✕ propagates an error from a noop error boundary during partial deferred mounting ✕ propagates an error from a noop error boundary during animation mounting The observed behavior is that unstable_handleError() unexpected gets called twice: "ErrorBoundary render success", "BrokenRender", "ErrorBoundary unstable_handleError", + "ErrorBoundary render success", + "BrokenRender", + "ErrorBoundary unstable_handleError", "ErrorBoundary render error" * Verify batched updates get scheduled despite errors * Add try/catch/finally blocks around commit phase passes We'll consolidate all these blocks in a future PR that refactors the commit phase to be separate from the perform work loop. * NoopBoundary -> RethrowBoundary * Only throw uncaught error once there is no more work to perform * Remove outdated comment It was fixed in facebook#8451. * Record tests * Always reset nextUnitOfWork on error This is important so that the test at the end of performAndHandleErrors() knows it's safe to rethrow. * Add a passing test for unmounting behavior on crashed tree * Top-level errors An error thrown from a host container should be "captured" by the host container itself * Remove outdated comment * Separate Rethrow and Noop scenarios in boundary tests * Move try block outside the commit loops

facebook-github-bot added GH Review: review-needed CLA Signed labels Nov 16, 2016

acdlite force-pushed the fibererroreffect branch from 0840bb1 to 3a75250 Compare November 16, 2016 17:15

facebook-github-bot added the CLA Signed label Nov 16, 2016

acdlite force-pushed the fibererroreffect branch from 3a75250 to 4a023c8 Compare November 16, 2016 17:19

facebook-github-bot added the CLA Signed label Nov 16, 2016

acdlite changed the title ~~[WIP][Fiber] New error boundary semantics~~ [Fiber] New error boundary semantics Nov 17, 2016

acdlite assigned gaearon and sebmarkbage Nov 17, 2016

acdlite force-pushed the fibererroreffect branch from 4e8844c to ffdc8cd Compare November 17, 2016 09:51

sebmarkbage reviewed Nov 17, 2016

View reviewed changes

acdlite force-pushed the fibererroreffect branch 2 times, most recently from 7e8791b to a8703e6 Compare November 17, 2016 13:31

acdlite mentioned this pull request Nov 17, 2016

[Fiber] Clear existing text content before inserting children #8331

Merged

sebmarkbage mentioned this pull request Nov 17, 2016

[Fiber] Umbrella for remaining features / bugs #7925

Closed

acdlite force-pushed the fibererroreffect branch from a8703e6 to f01a6e0 Compare November 17, 2016 16:29

gaearon reviewed Nov 17, 2016

View reviewed changes

gaearon previously requested changes Nov 17, 2016

View reviewed changes

acdlite force-pushed the fibererroreffect branch 2 times, most recently from 1745719 to f64ff30 Compare November 18, 2016 10:17

gaearon reviewed Nov 18, 2016

View reviewed changes

gaearon approved these changes Nov 18, 2016

View reviewed changes

gaearon previously requested changes Nov 18, 2016

View reviewed changes

sebmarkbage reviewed Nov 22, 2016

View reviewed changes

sebmarkbage added the GH Review: needs-revision label Nov 22, 2016

acdlite and others added 10 commits November 30, 2016 17:14

Add test for "unwinding" context when an error interrupts rendering

614cd4b

Switch over primary effect types only

4f126f5

This avoids the need to create an export for every combination of bits.

Only continue the work loop if the error was successfully captured

a61de42

Add try/catch/finally blocks around commit phase passes

fad5cb5

We'll consolidate all these blocks in a future PR that refactors the commit phase to be separate from the perform work loop.

NoopBoundary -> RethrowBoundary

ece9f7b

Only throw uncaught error once there is no more work to perform

226ee8f

Verify batched updates get scheduled despite errors

d27cada

Remove outdated comment

cc60f25

It was fixed in facebook#8451.

Record tests

778183e

gaearon force-pushed the fibererroreffect branch from b6d6017 to 778183e Compare November 30, 2016 17:19

gaearon and others added 5 commits November 30, 2016 18:31

Always reset nextUnitOfWork on error

54978e9

This is important so that the test at the end of performAndHandleErrors() knows it's safe to rethrow.

Add a passing test for unmounting behavior on crashed tree

c1f2eb2

Top-level errors

46581a3

An error thrown from a host container should be "captured" by the host container itself

Remove outdated comment

b32c86e

Separate Rethrow and Noop scenarios in boundary tests

7bc10c0

gaearon force-pushed the fibererroreffect branch from 8049226 to 7bc10c0 Compare November 30, 2016 19:53

sebmarkbage reviewed Nov 30, 2016

View reviewed changes

Move try block outside the commit loops

cf20dfa

gaearon approved these changes Nov 30, 2016

View reviewed changes

sebmarkbage reviewed Nov 30, 2016

View reviewed changes

sebmarkbage approved these changes Dec 1, 2016

View reviewed changes

sebmarkbage added GH Review: accepted and removed GH Review: review-needed labels Dec 1, 2016

acdlite merged commit e7eed58 into facebook:master Dec 1, 2016

This was referenced Dec 1, 2016

[Fiber] Add more tests and fix an issue with incremental error handling #8212

Closed

[Fiber] Force rerender a failed tree #8227

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fiber] New error boundary semantics #8304

[Fiber] New error boundary semantics #8304

acdlite commented Nov 16, 2016 •

edited

Loading

acdlite commented Nov 17, 2016

sebmarkbage Nov 17, 2016

gaearon Nov 17, 2016

gaearon left a comment

acdlite commented Nov 17, 2016

gaearon Nov 18, 2016

gaearon Nov 18, 2016

gaearon Nov 18, 2016

gaearon Nov 18, 2016

gaearon left a comment

gaearon left a comment

sebmarkbage Nov 22, 2016

acdlite Nov 28, 2016

acdlite Nov 28, 2016

sebmarkbage Nov 30, 2016

acdlite Nov 30, 2016

sebmarkbage Nov 30, 2016

acdlite Nov 30, 2016

gaearon left a comment

sebmarkbage Nov 30, 2016

acdlite Nov 30, 2016

sebmarkbage Nov 30, 2016

acdlite Nov 30, 2016

acdlite Nov 30, 2016

sebmarkbage left a comment

acdlite commented Dec 1, 2016

		@@ -54,8 +54,8 @@ describe('ReactIncrementalErrorHandling', () => {

		it('propagates an error from a noop error boundary', () => {
		class NoopBoundary extends React.Component {

[Fiber] New error boundary semantics #8304

[Fiber] New error boundary semantics #8304

Conversation

acdlite commented Nov 16, 2016 • edited Loading

acdlite commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

acdlite commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage left a comment

Choose a reason for hiding this comment

acdlite commented Dec 1, 2016

acdlite commented Nov 16, 2016 •

edited

Loading