[Fiber] Make error handling more resilient #8210
Conversation
* Ensure that errors in one root don't prevent work on another root * Fix an issue where boundary state change would get ignored in incremental mode
| @@ -1465,27 +1465,4 @@ describe('ReactIncremental', () => { | |||
| ]); | |||
| expect(instance.state.n).toEqual(4); | |||
| }); | |||
|
|
|||
| it('propagates an error from a noop error boundary', () => { | |||
There was a problem hiding this comment.
This was just moved into another file.
| @@ -561,47 +603,28 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) { | |||
| try { | |||
| performTaskWorkUnsafe(); | |||
| } catch (error) { | |||
| isPerformingTaskWork = false; | |||
There was a problem hiding this comment.
I moved this one into a try/finally in performTaskWorkUnsafe so that it's only set there.
There was a problem hiding this comment.
I agree it's easier to follow this way but I don't think it's worth a perf hit
| scheduleCallbackAtPriority(nextPriorityLevel); | ||
| } | ||
| } finally { | ||
| isPerformingTaskWork = false; |
There was a problem hiding this comment.
Functions that contain a try-catch/finally can't be optimized in V8. Can we move this reset back to the catch in handleErrors?
There was a problem hiding this comment.
Usual strategy is to split the try-catch/finally block into a separate function that does nothing but an unsafe function. We should do a pass at some point to do this across the codebase.
There was a problem hiding this comment.
I know but we're completely moving this code around every week. I'm worried microoptimizations just make it hard to follow right now. Let's do a pass as soon as it stabilizes a little bit?
There was a problem hiding this comment.
Maybe drop a TODO in there?
There was a problem hiding this comment.
👍 Could you please?
There was a problem hiding this comment.
When we do it, we should do it consistently IMO, not in an ad hoc way. Use a tryCatch helper like Bluebird or something like this.
| trapError(nextUnitOfWork, error, false); | ||
| } | ||
| } | ||
|
|
||
| // Unschedule the roots that fataled so that we can later schedule | ||
| // other updates to the same roots and potentially replace them. |
There was a problem hiding this comment.
Can you explain in more detail why it's necessary to unschedule failed roots?
There was a problem hiding this comment.
I want to continue working on the other roots in the same frame even if some failed, and only throw errors after all work is done (or frame is over). If I don't unschedule failed roots, findNextUnitOfWork will find them again, and we'll get into an infinite loop handling their errors again, then finding them again, and so on.
There was a problem hiding this comment.
I also don't want to get errors about a root I rendered once every time I schedule an update to another unrelated root. So I need it to be gone from the list until I explicitly retry it. You can see the desired behavior in the last test I added.
| }); | ||
| } | ||
|
|
||
| function unscheduleWork(unscheduledRoots : Set<FiberRoot>) { |
There was a problem hiding this comment.
I wonder if we had a way to only perform work on a specific root if we could avoid having to unschedule.
There was a problem hiding this comment.
I suspect this might not be too challenging. Add the ability to pass a root to perform* functions. If so, they won't attempt to look for new work once they run out.
There was a problem hiding this comment.
How would I know which root to work on? I only know which roots not to work on.
|
Going to try to remove the need for unscheduling for a bit, otherwise I'll merge as-is |
|
👍 Leaving it to you. |
We can use the existing unscheduling logic in findNextUnitOfWork.
Supersedes #8166.