Skip to content

Commit

Permalink
[scheduler] 5/n Error handling in scheduler (#12920)
Browse files Browse the repository at this point in the history 
* Initial failing unit test for error handling in schedule

**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!

* Add fixture for testing error handling in scheduler

**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.

* Handle errors in scheduler

**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.

* Add more tests for error handling in scheduler

**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.

* callSafely -> callUnsafely

* Fix bugs with error handling in schedule

**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.

* Unit tests for error handling with timed out callbacks

**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.

* Adds fixture to test timed out callbacks with scheduler

**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.

* fix rebase problems

* make fixture compensate for chrome JS speed

* ran prettier

* Remove 'cancelled' flag on callbackConfig in scheduler, add test

**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.

* fix typo
  • Loading branch information
@flarnie
flarnie authored May 30, 2018
1 parent 3118ed9 commit 15767a8
Show file tree
Hide file tree
Showing 3 changed files with 624 additions and 30 deletions.
178 changes: 177 additions & 1 deletion fixtures/schedule/index.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,16 @@ <h2>Tests:</h2>
<div><b>Actual:</b></div>
<div id="test-4"></div>
</li>
<li>
<p>When some callbacks throw errors, still calls them all within the same frame</p>
<p><b>IMPORTANT:</b> Open the console when you run this! Inspect the logs there!</p>
<button onClick="runTestFive()">Run Test 5</button>
</li>
<li>
<p>When some callbacks throw errors <b> and some also time out</b>, still calls them all within the same frame</p>
<p><b>IMPORTANT:</b> Open the console when you run this! Inspect the logs there!</p>
<button onClick="runTestSix()">Run Test 6</button>
</li>
</ol>
<script src="../../build/dist/react-scheduler.development.js"></script>
<script src="https://unpkg.com/babel-standalone@6/babel.js"></script>
Expand Down Expand Up @@ -134,6 +144,9 @@ <h2>Tests:</h2>
// test 4
[
],
// test 5
[
],
];

const expectedResults = [
Expand Down Expand Up @@ -182,6 +195,10 @@ <h2>Tests:</h2>
'cbD called with argument of {"didTimeout":false}',
'frame 3 started... we stop counting now.',
],
// test 5
[
// ... TODO
],
];
function runTestOne() {
// Test 1
Expand Down Expand Up @@ -276,7 +293,7 @@ <h2>Tests:</h2>
updateTestResult(4, 'scheduled cbA');
scheduleWork(cbB, {timeout: 100}); // times out later
updateTestResult(4, 'scheduled cbB');
scheduleWork(cbC, {timeout: 2}); // will time out fast
scheduleWork(cbC, {timeout: 1}); // will time out fast
updateTestResult(4, 'scheduled cbC');
scheduleWork(cbD); // won't time out
updateTestResult(4, 'scheduled cbD');
Expand All @@ -287,6 +304,165 @@ <h2>Tests:</h2>
displayTestResult(4);
checkTestResult(4);
});

}

// Error handling

function runTestFive() {
// Test 5
// When some callbacks throw errors, still calls them all within the same frame
const cbA = (x) => {
console.log('cbA called with argument of ' + JSON.stringify(x));
}
const cbB = (x) => {
console.log('cbB called with argument of ' + JSON.stringify(x));
console.log('cbB is about to throw an error!');
throw new Error('error B');
}
const cbC = (x) => {
console.log('cbC called with argument of ' + JSON.stringify(x));
}
const cbD = (x) => {
console.log('cbD called with argument of ' + JSON.stringify(x));
console.log('cbD is about to throw an error!');
throw new Error('error D');
}
const cbE = (x) => {
console.log('cbE called with argument of ' + JSON.stringify(x));
console.log('This was the last callback! ------------------');
}

console.log('We are aiming to roughly emulate the way ' +
'`requestAnimationFrame` handles errors from callbacks.');

console.log('about to run the simulation of what it should look like...:');

requestAnimationFrame(() => {
console.log('frame 1 started');
requestAnimationFrame(() => {
console.log('frame 2 started');
requestAnimationFrame(() => {
console.log('frame 3 started... we stop counting now.');
console.log('about to wait a moment and start this again but ' +
'with the scheduler instead of requestAnimationFrame');
setTimeout(runSchedulerCode, 1000);
});
});
});
requestAnimationFrame(cbA);
console.log('scheduled cbA');
requestAnimationFrame(cbB); // will throw error
console.log('scheduled cbB');
requestAnimationFrame(cbC);
console.log('scheduled cbC');
requestAnimationFrame(cbD); // will throw error
console.log('scheduled cbD');
requestAnimationFrame(cbE);
console.log('scheduled cbE');


function runSchedulerCode() {
console.log('-------------------------------------------------------------');
console.log('now lets see what it looks like using the scheduler...:');
requestAnimationFrame(() => {
console.log('frame 1 started');
requestAnimationFrame(() => {
console.log('frame 2 started');
requestAnimationFrame(() => {
console.log('frame 3 started... we stop counting now.');
});
});
});
scheduleWork(cbA);
console.log('scheduled cbA');
scheduleWork(cbB); // will throw error
console.log('scheduled cbB');
scheduleWork(cbC);
console.log('scheduled cbC');
scheduleWork(cbD); // will throw error
console.log('scheduled cbD');
scheduleWork(cbE);
console.log('scheduled cbE');
};
}

function runTestSix() {
// Test 6
// When some callbacks throw errors, still calls them all within the same frame
const cbA = (x) => {
console.log('cbA called with argument of ' + JSON.stringify(x));
console.log('cbA is about to throw an error!');
throw new Error('error A');
}
const cbB = (x) => {
console.log('cbB called with argument of ' + JSON.stringify(x));
}
const cbC = (x) => {
console.log('cbC called with argument of ' + JSON.stringify(x));
}
const cbD = (x) => {
console.log('cbD called with argument of ' + JSON.stringify(x));
console.log('cbD is about to throw an error!');
throw new Error('error D');
}
const cbE = (x) => {
console.log('cbE called with argument of ' + JSON.stringify(x));
console.log('This was the last callback! ------------------');
}

console.log('We are aiming to roughly emulate the way ' +
'`requestAnimationFrame` handles errors from callbacks.');

console.log('about to run the simulation of what it should look like...:');

requestAnimationFrame(() => {
console.log('frame 1 started');
requestAnimationFrame(() => {
console.log('frame 2 started');
requestAnimationFrame(() => {
console.log('frame 3 started... we stop counting now.');
console.log('about to wait a moment and start this again but ' +
'with the scheduler instead of requestAnimationFrame');
setTimeout(runSchedulerCode, 1000);
});
});
});
requestAnimationFrame(cbC);
console.log('scheduled cbC first; simulating timing out');
requestAnimationFrame(cbD); // will throw error
console.log('scheduled cbD first; simulating timing out');
requestAnimationFrame(cbE);
console.log('scheduled cbE first; simulating timing out');
requestAnimationFrame(cbA);
console.log('scheduled cbA'); // will throw error
requestAnimationFrame(cbB);
console.log('scheduled cbB');


function runSchedulerCode() {
console.log('-------------------------------------------------------------');
console.log('now lets see what it looks like using the scheduler...:');
requestAnimationFrame(() => {
console.log('frame 1 started');
requestAnimationFrame(() => {
console.log('frame 2 started');
requestAnimationFrame(() => {
console.log('frame 3 started... we stop counting now.');
});
});
});
scheduleWork(cbA);
console.log('scheduled cbA');
scheduleWork(cbB); // will throw error
console.log('scheduled cbB');
scheduleWork(cbC, {timeout: 1});
console.log('scheduled cbC');
scheduleWork(cbD, {timeout: 1}); // will throw error
console.log('scheduled cbD');
scheduleWork(cbE, {timeout: 1});
console.log('scheduled cbE');
};
}
</script type="text/babel">
</body>
Expand Down
89 changes: 63 additions & 26 deletions packages/react-scheduler/src/ReactScheduler.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,33 @@ if (!ExecutionEnvironment.canUseDOM) {
},
};

/**
* Handles the case where a callback errors:
* - don't catch the error, because this changes debugging behavior
* - 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 callUnsafely = function(
callbackConfig: CallbackConfigType,
arg: Deadline,
) {
const callback = callbackConfig.scheduledCallback;
let finishedCalling = false;
try {
callback(arg);
finishedCalling = true;
} finally {
// always remove it from linked list
cancelScheduledWork(callbackConfig);

if (!finishedCalling) {
// an error must have been thrown
isIdleScheduled = true;
window.postMessage(messageKey, '*');
}
}
};

/**
* Checks for timed out callbacks, runs them, and then checks again to see if
* any more have timed out.
Expand All @@ -152,32 +179,42 @@ if (!ExecutionEnvironment.canUseDOM) {
// We know that none of them have timed out yet.
return;
}
nextSoonestTimeoutTime = -1; // we will reset it below

// keep checking until we don't find any more timed out callbacks
frameDeadlineObject.didTimeout = true;
// NOTE: we intentionally wait to update the nextSoonestTimeoutTime until
// after successfully calling any timed out callbacks.
// If a timed out callback throws an error, we could get stuck in a state
// where the nextSoonestTimeoutTime was set wrong.
let updatedNextSoonestTimeoutTime = -1; // we will update nextSoonestTimeoutTime below
const timedOutCallbacks = [];

// iterate once to find timed out callbacks and find nextSoonestTimeoutTime
let currentCallbackConfig = headOfPendingCallbacksLinkedList;
while (currentCallbackConfig !== null) {
const timeoutTime = currentCallbackConfig.timeoutTime;
if (timeoutTime !== -1 && timeoutTime <= currentTime) {
// it has timed out!
// call it
const callback = currentCallbackConfig.scheduledCallback;
// TODO: error handling
callback(frameDeadlineObject);
// remove it from linked list
cancelScheduledWork(currentCallbackConfig);
timedOutCallbacks.push(currentCallbackConfig);
} else {
if (
timeoutTime !== -1 &&
(nextSoonestTimeoutTime === -1 ||
timeoutTime < nextSoonestTimeoutTime)
(updatedNextSoonestTimeoutTime === -1 ||
timeoutTime < updatedNextSoonestTimeoutTime)
) {
nextSoonestTimeoutTime = timeoutTime;
updatedNextSoonestTimeoutTime = timeoutTime;
}
}
currentCallbackConfig = currentCallbackConfig.next;
}

if (timedOutCallbacks.length > 0) {
frameDeadlineObject.didTimeout = true;
for (let i = 0, len = timedOutCallbacks.length; i < len; i++) {
callUnsafely(timedOutCallbacks[i], frameDeadlineObject);
}
}

// NOTE: we intentionally wait to update the nextSoonestTimeoutTime until
// after successfully calling any timed out callbacks.
nextSoonestTimeoutTime = updatedNextSoonestTimeoutTime;
};

// We use the postMessage trick to defer idle work until after the repaint.
Expand Down Expand Up @@ -206,20 +243,9 @@ if (!ExecutionEnvironment.canUseDOM) {
headOfPendingCallbacksLinkedList !== null
) {
const latestCallbackConfig = headOfPendingCallbacksLinkedList;
// move head of list to next callback
headOfPendingCallbacksLinkedList = latestCallbackConfig.next;
if (headOfPendingCallbacksLinkedList !== null) {
headOfPendingCallbacksLinkedList.prev = null;
} else {
// if headOfPendingCallbacksLinkedList is null,
// then the list must be empty.
// make sure we set the tail to null as well.
tailOfPendingCallbacksLinkedList = null;
}
frameDeadlineObject.didTimeout = false;
const latestCallback = latestCallbackConfig.scheduledCallback;
// TODO: before using this outside of React we need to add error handling
latestCallback(frameDeadlineObject);
// callUnsafely will remove it from the head of the linked list
callUnsafely(latestCallbackConfig, frameDeadlineObject);
currentTime = now();
}
if (headOfPendingCallbacksLinkedList !== null) {
Expand Down Expand Up @@ -315,6 +341,15 @@ if (!ExecutionEnvironment.canUseDOM) {
cancelScheduledWork = function(
callbackConfig: CallbackIdType /* CallbackConfigType */,
) {
if (
callbackConfig.prev === null &&
headOfPendingCallbacksLinkedList !== callbackConfig
) {
// this callbackConfig has already been cancelled.
// cancelScheduledWork should be idempotent, a no-op after first call.
return;
}

/**
* There are four possible cases:
* - Head/nodeToRemove/Tail -> null
Expand All @@ -331,6 +366,8 @@ if (!ExecutionEnvironment.canUseDOM) {
*/
const next = callbackConfig.next;
const prev = callbackConfig.prev;
callbackConfig.next = null;
callbackConfig.prev = null;
if (next !== null) {
// we have a next

Expand Down
Loading

0 comments on commit 15767a8

Please sign in to comment.