[scheduler] 5/n Error handling in scheduler #12920
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
ReactDOM: size: 🔺+0.2%, gzip: 🔺+0.2% Details of bundled changes.Comparing: 79a740c...a7b13aa react-dom
react-art
react-scheduler
Generated by 🚫 dangerJS |
mihalskiy
approved these changes
May 28, 2018
acdlite
reviewed
May 29, 2018
| * - do start a new postMessage callback, to call any remaining callbacks, | ||
| * - but only if there is an error, so there is not extra overhead. | ||
| */ | ||
| const callSafely = function(callback, arg) { |
There was a problem hiding this comment.
callSafely implies that the function won't throw. Can we rename this to callUnsafely or similar?
| @@ -155,8 +175,7 @@ if (!ExecutionEnvironment.canUseDOM) { | |||
| // it has timed out! | |||
| // call it | |||
| const callback = currentCallbackConfig.scheduledCallback; | |||
| // TODO: error handling | |||
| callback(frameDeadlineObject); | |||
| callSafely(callback, frameDeadlineObject); | |||
There was a problem hiding this comment.
A timed out callback that throws will infinite loop, because it never gets cancelled by the line below. Need unit test.
**what is the change?:** see title **why make this change?:** Adding tests for the error handling behavior we are about to add. This test is failing, which gives us the chance to make it pass. Wrote skeletons of some other tests to add. Unit testing this way is really hacky, and I'm also adding to the fixture to test this in the real browser environment. **test plan:** Ran new test, saw it fail!
**what is the change?:** Added a fixture which does the following - logs in the console to show what happens when you use `requestAnimationFrame` to schedule a series of callbacks and some of them throw errors. Then does the same actions with the `scheduler` and verifies that it behaves in a similar way. Hard to really verify the errors get thrown at the proper time without looking at the console. **why make this change?:** We want the most authentic, accurate test of how errors are handled in the scheduler. That's what this fixture should be. **test plan:** Manually verified that this test does what I expect - right now it's failing but follow up commits will fix that.
**what is the change?:** We set a flag before calling any callback, and then use a 'try/finally' block to wrap it. Note that we *do not* catch the error, if one is thrown. But, we only unset the flag after the callback successfully finishes. If we reach the 'finally' block and the flag was not unset, then it means an error was thrown. In that case we start a new postMessage callback, to finish calling any other pending callbacks if there is time. **why make this change?:** We need to make sure that an error thrown from one callback doesn't stop other callbacks from firing, but we also don't want to catch or swallow the error because we want engineers to still be able to log and debug errors. **test plan:** New tests added are passing, and we verified that they fail without this change.
**what is the change?:** Added tests for more situations where error handling may come up. **why make this change?:** To get additional protection against this being broken in the future. **test plan:** Ran new tests and verified that they fail when error handling fails.
**what is the change?:** - ensure that we properly remove the callback from the linked list, even if it throws an error and is timed out. - ensure that you can call 'cancelScheduledWork' more than once and it is idempotent. **why make this change?:** To fix bugs :) **test plan:** Existing tests pass, and we'll add more tests in a follow up commit.
**what is the change?:** More unit tests, to cover behavior which we missed; error handling of timed out callbacks. **why make this change?:** To protect the future!~ **test plan:** Run the new tests.
**what is the change?:** See title In the other error handling fixture we compare 'scheduleWork' error handling to 'requestAnimationFrame' and try to get as close as possible. There is no 'timing out' feature with 'requestAnimationFrame' but effectively the 'timing out' feature changes the order in which things are called. So we just changed the order in the 'requestAnimationFrame' version and that works well for illustrating the behavior we expect in the 'scheduleWork' test. **why make this change?:** We need more test coverage of timed out callbacks. **test plan:** Executed the fixture manually in Firefox and Chrome. Results looked good.
flarnie
force-pushed
the
errorHandlingInScheduler1
branch
from
e60db4f to
8759d56
Compare
|
Minor TODOs for tomorrow morning - |
|
:( forgot prettier Please still review 😇 |
acdlite
reviewed
May 30, 2018
| } | ||
|
|
||
| // cancelScheduledWork should be idempotent, a no-op after first call. | ||
| callbackConfig.cancelled = true; |
There was a problem hiding this comment.
This doesn't seem necessary. Could you replace with something like
const alreadyCancelled = callbackConfig.next === null && callbackConfig !== tail;?
**what is the change?:** - Instead of using a 'cancelled' flag on the callbackConfig, it's easier to just check the state of the callbackConfig inside 'cancelScheduledWork' to determine if it's already been cancelled. That way we don't have to remember to set the 'cancelled' flag every time we call a callback or cancel it. One less thing to remember. - We added a test for calling 'cancelScheduledWork' more than once, which would have failed before. Thanks @acdlite for suggesting this in code review. :) **why make this change?:** To increase stability of the schedule module, increase test coverage. **test plan:** Existing tests pass and we added a new test to cover this behavior.
| * throw errors | ||
| * | ||
| * | ||
| */ |
There was a problem hiding this comment.
I'm a little confused about the graph, if you call poseMessage, the onmessage handler is fired after microtask and before rAF, but this graph shows it will be fired after rAF, why?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
what is the change?:
We set a flag before calling any callback, and then use a 'try/finally'
block to wrap it. Note that we do not catch the error, if one is
thrown. But, we only unset the flag after the callback successfully
finishes.
If we reach the 'finally' block and the flag was not unset, then it
means an error was thrown.
In that case we start a new postMessage callback, to finish calling any
other pending callbacks if there is time.
why make this change?:
We need to make sure that an error thrown from one callback doesn't stop
other callbacks from firing, but we also don't want to catch or swallow
the error because we want engineers to still be able to log and debug
errors. And we don't want to push errors into the wrong frame or change their timing - this simulates almost exactly how it would work with 'requestAnimationFrame' if the callback throws an error.
test plan:
New tests added are passing, and we verified that they fail without this
change.
In particular, the fixture shows how errors and callbacks act in a real browser and compares the 'schedule.scheduleWork' method to 'requestAnimationFrame'. In Chrome the results are the same:
Edit: In Firefox they are also the same;
Haven't tested IE/Edge yet but I will, not expecting problems there.