[Fiber] Full Error Boundaries #8095

gaearon · 2016-10-25T18:45:09Z

Picking up the work I dropped in #7993.

Now this passes all existing tests, including those that don't work in stack reconciler (setState, didMount, and didUpdate).

sebmarkbage · 2016-10-25T18:54:01Z

src/renderers/shared/fiber/ReactFiberCommitWork.js

+  function commitUnmount(current : Fiber, safely : boolean) : void {
+    // Make sure we mark this deletion as completed so that we don't
+    // attempt to perform it again if one of the deletions fails, and
+    // error boundary tries to unmount this subtree again.


Can you explain how this happens? When we commit, we've already swapped the tree so as far as Fiber is concerned, it is deleted in the "current" state. So any rerender should presumably not try to unmount again.

When we commit, we've already swapped the tree so as far as Fiber is concerned, it is deleted in the "current" state.

Hmm, I think this is something I changed below:

commitAllWork(workInProgress, isHandlingError); // Swap the pointer after committing all work so that if committing fails, // we still treat it as a work in progress in case there is an error boundary. root.current = workInProgress;

Without changing it, Fiber complained that

Cannot commit the same tree as before. This is probably a bug related to the return field.

whenever I caught an error during commit phase (e.g. DidMount) and tried to get error boundary to re-render the whole thing.

Alternatively I could "roll the pointer back" on error but I didn't want a try catch there.
What should I do?

So it's the same problem as was solved by calledCompobentWillUnmount in the old reconciler.

We need to recover if we committed some unmounts, one of them failed, and we need to safely unmount the rest without repeating failed ones.

It does look like you're tracking the currently unmounting Fiber anywhere so how do you know which error boundary to invoke if componentWillUnmount throws during a normal unmount?

In theory, since you need to track the currently unmounting Fiber you should be able to know which one failed. Since only one can fail, you can continue walking the tree from that point on. You'd know that anything after the failing node hasn't yet unmounted and anything before it has unmounted.

sebmarkbage · 2016-10-25T18:58:21Z

src/renderers/shared/fiber/ReactFiberCommitWork.js

@@ -155,15 +156,15 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
    }
  }

-  function commitNestedUnmounts(root : Fiber) {
+  function commitNestedUnmounts(root : Fiber, safely : boolean) {


In ReactChildFiber, I used a flag in a closure. The idea is that it makes it easy to just clone this to two separate paths. That way, in the normal case, we don't have to copy this boolean between stack frames and check its value at every stop.

sebmarkbage · 2016-10-25T20:38:08Z

src/renderers/shared/fiber/ReactFiberScheduler.js

+        commitAllWork(workInProgress, isHandlingError);
+        // Swap the pointer after committing all work so that if committing fails,
+        // we still treat it as a work in progress in case there is an error boundary.
+        root.current = workInProgress;


This probably makes the most sense but I'm worried about how this recovery works.

I make a lot of assumptions about how this works and if something is partially committed I'm not sure if it is safe to continue from the "current" tree. Will it try to do deletions and insertions again? We might be able to reset those flags so that we can skip them.

In React Native we rely on things like indices so that we don't have to read from the native tree. Once we have already sent off a mutation, it's too late to go back on it.

We might need to guarantee that at least all host mutations are committed.

sebmarkbage · 2016-10-25T20:47:21Z

src/renderers/shared/fiber/ReactFiberScheduler.js

+  function revertLifeCyclesCommittedSoFar(finishedWork : Fiber, failedEffect : Fiber) {
+    // Gather effects in an array because this is a rare code path.
+    // We need to call them in the reverse order.
+    const fibersCommittedSoFar = [];


Would it be possible to reuse the nextEffect pointer here since you probably don't need it anymore? In that case, it's just a "reversing a singly linked list" problem.

sebmarkbage · 2016-10-25T20:52:36Z

src/renderers/shared/fiber/ReactFiberScheduler.js

@@ -133,7 +133,8 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
          break;
        }
        case Deletion: {
-          commitDeletion(effectfulFiber);
+          const safely = isHandlingError;
+          commitDeletion(effectfulFiber, safely);


Just an observation that this is the only thing in the first pass that might throw.

sebmarkbage · 2016-10-27T04:04:35Z

Question: Is the error boundary forced to unmount all its children if an error happens? And then render something else?

If not, how do we know they don't need to clean up the failed update? If we force componentWillUnmount then that is the hook to that enables that clean up. If not, then maybe we need something like componentWillRecover that lets it handle recovery - but will anyone implement that?

iamdustan · 2016-10-27T18:07:54Z

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

-    expect(() => {
+  if (ReactDOMFeatureFlags.useFiber) {
+    // This test implements a new feature in Fiber.
+    xit('catches errors in componentDidMount', () => {


this intentionally still xit?

iamdustan · 2016-10-27T18:09:00Z

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

+    });
+  }
+
+  fit('recovers from componentWillUnmount errors on update', () => {


...and fit?

It's all broken now :-) I'm just pushing to make myself feel better. I'll push the final version when it's ready.

gaearon · 2016-10-27T22:54:15Z

@sebmarkbage

Pushed a commit with new semantics: we treat commits as atomic and instead of trying to "revert" lifecycle hooks we isolate each of them, and leave error handling until after commit.

The tests are passing but this is missing the case where independent error boundaries get notified about their updates separately. I'll add a failing test for this and try to figure it out. For now, I only handle the first error but I should have enough information to handle the first error per each boundary separately.

I'm not very happy about the way error handling is nested inside the commit phase. Maybe we can put trapped errors as a top level variable and handle them between normal work items as part of finding the next unit or something. Maybe #8127 would help too.

I'd like your feedback on the overall approach here. If this looks okay maybe merge it as is and I'll do the rest on top?

sebmarkbage · 2016-10-27T23:35:52Z

Awesome! I'll take a look tonight.

sebmarkbage

Ok, looks good. I think this is in a state that we can land this as the first version.

sebmarkbage · 2016-10-28T03:55:36Z

src/renderers/shared/fiber/ReactFiberScheduler.js

@@ -267,7 +300,7 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
        // "next" scheduled work since we've already scanned passed. That
        // also ensures that work scheduled during reconciliation gets deferred.
        // const hasMoreWork = workInProgress.pendingWorkPriority !== NoWork;
-        commitAllWork(workInProgress);
+        commitAllWork(workInProgress, ignoreUnmountingErrors);


FWIW, the fact that commit happens here is an unfortunate artifact. When the last child completes, it have to backtrack quite a bit. Then finally it have to commit all work. All without checking remainingTime(). So we're actually quite likely to push into the next frame here. Ideally commitAllWork should move out so that the commit can happen in the beginning of the next requestIdleCallback instead of at the end of this one. That would make it so we don't have to pass this flag along.

sebmarkbage · 2016-10-28T03:56:44Z

src/renderers/shared/fiber/ReactFiberCommitWork.js

      }
      default:
        throw new Error('This unit of work tag should not have side-effects.');
    }
  }

+  function tryCallComponentDidMount(instance) {


I believe there is a way we can rewrite the commit phase so that we don't have to do nested try catches deep in it but this is fine as the first version.

sebmarkbage · 2016-10-28T03:59:01Z

src/renderers/shared/fiber/ReactFiberScheduler.js

+    } catch (nextError) {
+      // Propagate error to the next boundary or rethrow.
+      const nextTrappedError = trapError(boundary, nextError);
+      handleError(nextTrappedError);


This is the only place we have recursion. Kind of unfortunate.

sebmarkbage · 2016-10-28T04:11:44Z

src/renderers/shared/fiber/ReactFiberScheduler.js

+
+    // Now that the tree has been committed, we can handle errors.
+    if (allTrappedErrors) {
+      // TODO: handle multiple errors with distinct boundaries.


A plausible strategy: When an error happens you can keep walking through the unmounting children and the effect list until you hit the lastEffect edge of the boundary. Then you can handle the error right there without keeping an array of them. Then you continue committing the rest.

However, then the tree wouldn't be fully committed in unstable_handleError which is probably bad. So the array strategy might be needed after all.

gaearon assigned sebmarkbage Oct 25, 2016

facebook-github-bot added GH Review: review-needed CLA Signed labels Oct 25, 2016

sebmarkbage reviewed Oct 25, 2016

View reviewed changes

sebmarkbage mentioned this pull request Oct 26, 2016

[Fiber] String refs and owner tracking #8099

Merged

gaearon force-pushed the fiber-error-boundaries-take-two branch from 53c3212 to 26ccc8f Compare October 26, 2016 15:01

iamdustan reviewed Oct 27, 2016

View reviewed changes

gaearon force-pushed the fiber-error-boundaries-take-two branch from 1768e71 to 352bf6d Compare October 27, 2016 22:49

[Fiber] Error Boundaries

57c5acd

gaearon force-pushed the fiber-error-boundaries-take-two branch from 352bf6d to 57c5acd Compare October 27, 2016 22:57

sebmarkbage approved these changes Oct 28, 2016

View reviewed changes

sebmarkbage added GH Review: accepted and removed GH Review: review-needed labels Oct 28, 2016

gaearon merged commit 3c6abbf into facebook:master Oct 28, 2016

gaearon deleted the fiber-error-boundaries-take-two branch October 28, 2016 13:16

gaearon mentioned this pull request Oct 30, 2016

[Fiber] Remove recursion from error handling #8153

Closed

acusti pushed a commit to brandcast/react that referenced this pull request Mar 15, 2017

[Fiber] Error Boundaries (facebook#8095)

40dd651

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fiber] Full Error Boundaries #8095

[Fiber] Full Error Boundaries #8095

gaearon commented Oct 25, 2016

sebmarkbage Oct 25, 2016

gaearon Oct 25, 2016 •

edited

Loading

gaearon Oct 25, 2016

sebmarkbage Oct 25, 2016

sebmarkbage Oct 25, 2016

sebmarkbage Oct 25, 2016

sebmarkbage Oct 25, 2016

sebmarkbage Oct 25, 2016

sebmarkbage commented Oct 27, 2016

iamdustan Oct 27, 2016

iamdustan Oct 27, 2016

gaearon Oct 27, 2016

gaearon commented Oct 27, 2016

sebmarkbage commented Oct 27, 2016

sebmarkbage left a comment

sebmarkbage Oct 28, 2016

sebmarkbage Oct 28, 2016

sebmarkbage Oct 28, 2016

sebmarkbage Oct 28, 2016

[Fiber] Full Error Boundaries #8095

[Fiber] Full Error Boundaries #8095

Conversation

gaearon commented Oct 25, 2016

Choose a reason for hiding this comment

gaearon Oct 25, 2016 • edited Loading

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

sebmarkbage commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Oct 27, 2016

sebmarkbage commented Oct 27, 2016

sebmarkbage 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

gaearon Oct 25, 2016 •

edited

Loading