Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[scheduler] 5/n Error handling in scheduler #12920

Merged
merged 13 commits into from
May 30, 2018
Merged

[scheduler] 5/n Error handling in scheduler #12920

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b18a86c
Initial failing unit test for error handling in schedule
flarnie May 25, 2018
16972de
Add fixture for testing error handling in scheduler
flarnie May 26, 2018
0ac6e11
Handle errors in scheduler
flarnie May 28, 2018
be9bda6
Add more tests for error handling in scheduler
flarnie May 28, 2018
0dde6a1
callSafely -> callUnsafely
flarnie May 29, 2018
0743201
Fix bugs with error handling in schedule
flarnie May 29, 2018
0212f82
Unit tests for error handling with timed out callbacks
flarnie May 29, 2018
102d7c6
Adds fixture to test timed out callbacks with scheduler
flarnie May 29, 2018
5c0742a
fix rebase problems
flarnie May 30, 2018
8759d56
make fixture compensate for chrome JS speed
flarnie May 30, 2018
534a488
ran prettier
flarnie May 30, 2018
1ae920a
Remove 'cancelled' flag on callbackConfig in scheduler, add test
flarnie May 30, 2018
a7b13aa
fix typo
flarnie May 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
178 changes: 177 additions & 1 deletion fixtures/schedule/index.html
View file
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ type CallbackConfigType = {|
timeoutTime: number,
next: CallbackConfigType | null, // creating a linked list
prev: CallbackConfigType | null, // creating a linked list
cancelled: boolean,
|};

export type CallbackIdType = CallbackConfigType;
Expand Down Expand Up @@ -85,6 +86,7 @@ if (!ExecutionEnvironment.canUseDOM) {
timeoutTime: 0,
next: null,
prev: null,
cancelled: false,
};
const timeoutId = localSetTimeout(() => {
callback({
Expand All @@ -95,6 +97,7 @@ if (!ExecutionEnvironment.canUseDOM) {
});
});
timeoutIds.set(callback, timeoutId);
callbackConfig.cancelled = true;
return callbackConfig;
};
cancelScheduledWork = function(callbackId: CallbackIdType) {
Expand Down Expand Up @@ -129,6 +132,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 +182,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 +246,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 @@ -285,6 +314,7 @@ if (!ExecutionEnvironment.canUseDOM) {
timeoutTime,
prev: null,
next: null,
cancelled: false,
};
if (headOfPendingCallbacksLinkedList === null) {
// Make this callback the head and tail of our list
Expand Down Expand Up @@ -315,6 +345,13 @@ if (!ExecutionEnvironment.canUseDOM) {
cancelScheduledWork = function(
callbackConfig: CallbackIdType /* CallbackConfigType */,
) {
if (callbackConfig.cancelled === true) {
return;
}

// cancelScheduledWork should be idempotent, a no-op after first call.
callbackConfig.cancelled = true;
Copy link
Collaborator

@acdlite acdlite May 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem necessary. Could you replace with something like

const alreadyCancelled = callbackConfig.next === null && callbackConfig !== tail;

?


/**
* There are four possible cases:
* - Head/nodeToRemove/Tail -> null
Expand Down
Loading