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.

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] Re-throw unhandled errors #19595

Merged
merged 1 commit into from
Aug 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 16 additions & 17 deletions packages/scheduler/src/SchedulerPostTask.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export {

// Capture local references to native APIs, in case a polyfill overrides them.
const perf = window.performance;
const setTimeout = window.setTimeout;

// Use experimental Chrome Scheduler postTask API.
const scheduler = global.scheduler;
Expand Down Expand Up @@ -112,7 +113,7 @@ export function unstable_scheduleCallback<T>(
runTask.bind(null, priorityLevel, postTaskPriority, node, callback),
postTaskOptions,
)
.catch(handlePostTaskError);
.catch(handleAbortError);

return node;
}
Expand Down Expand Up @@ -150,30 +151,28 @@ function runTask<T>(
),
continuationOptions,
)
.catch(handlePostTaskError);
.catch(handleAbortError);
}
} catch (error) {
// We're inside a `postTask` promise. If we don't handle this error, then it
// will trigger an "Unhandled promise rejection" error. We don't want that,
// but we do want the default error reporting behavior that normal
// (non-Promise) tasks get for unhandled errors.
//
// So we'll re-throw the error inside a regular browser task.
setTimeout(() => {
throw error;
});
} finally {
currentPriorityLevel_DEPRECATED = NormalPriority;
}
}

function handlePostTaskError(error) {
// This error is either a user error thrown by a callback, or an AbortError
// as a result of a cancellation.
//
// User errors trigger a global `error` event even if we don't rethrow them.
// In fact, if we do rethrow them, they'll get reported to the console twice.
// I'm not entirely sure the current `postTask` spec makes sense here. If I
// catch a `postTask` call, it shouldn't trigger a global error.
//
function handleAbortError(error) {
Copy link
Member

@rickhanlonii rickhanlonii Aug 13, 2020

Choose a reason for hiding this comment

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

Doesn't handleAbortError assume that the only way postTask rejects is via an abort?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah. I'm pretty sure that's a correct assumption. Since the user code is wrapped in its own try/catch.

// Abort errors are an implementation detail. We don't expose the
// TaskController to the user, nor do we expose the promise that is returned
// from `postTask`. So we shouldn't rethrow those, either, since there's no
// way to handle them. (If we did return the promise to the user, then it
// should be up to them to handle the AbortError.)
//
// In either case, we can suppress the error, barring changes to the spec
// or the Scheduler API.
// from `postTask`. So we should suppress them, since there's no way for the
// user to handle them.
}

export function unstable_cancelCallback(node: CallbackNode) {
Expand Down
30 changes: 14 additions & 16 deletions packages/scheduler/src/__tests__/SchedulerPostTask-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,15 @@ describe('SchedulerPostTask', () => {
},
};

// Note: setTimeout is used to report errors and nothing else.
window.setTimeout = cb => {
try {
cb();
} catch (error) {
runtime.log(`Error: ${error.message}`);
}
};

// Mock browser scheduler.
const scheduler = {};
global.scheduler = scheduler;
Expand Down Expand Up @@ -116,16 +125,10 @@ describe('SchedulerPostTask', () => {
// delete the continuation task.
const prevTaskQueue = taskQueue;
taskQueue = new Map();
for (const [, {id, callback, resolve, reject}] of prevTaskQueue) {
try {
log(`Task ${id} Fired`);
callback(false);
resolve();
} catch (error) {
log(`Task ${id} errored [${error.message}]`);
reject(error);
continue;
}
for (const [, {id, callback, resolve}] of prevTaskQueue) {
log(`Task ${id} Fired`);
callback(false);
resolve();
}
}
function log(val) {
Expand Down Expand Up @@ -219,12 +222,7 @@ describe('SchedulerPostTask', () => {
'Post Task 1 [user-visible]',
]);
runtime.flushTasks();
runtime.assertLog([
'Task 0 Fired',
'Task 0 errored [Oops!]',
'Task 1 Fired',
'Yay',
]);
runtime.assertLog(['Task 0 Fired', 'Error: Oops!', 'Task 1 Fired', 'Yay']);
});

it('schedule new task after queue has emptied', () => {
Expand Down