[Scheduler] Re-throw unhandled errors (#19595)
Because `postTask` returns a promise, errors inside a `postTask` callback result in the promise being rejected. If we don't catch those errors, then the browser will report an "Unhandled promise rejection" error. This is a confusing message to see in the console, because the fact that `postTask` is a promise-based API is an implementation detail from the perspective of the developer. "Promise rejection" is a red herring. On the other hand, if we do catch those errors, then we need to report the error to the user in some other way. What we really want is the default error reporting behavior that a normal, non-Promise browser event gets. So, we'll re-throw inside `setTimeout`.
Andrew Clark committed
Aug 12, 2020 at 22:06 UTC
629125555f381764c9f4943e877985274ebbc1b6
2 files changed
+30
-33
packages/scheduler/src/SchedulerPostTask.js
+16
-17
@@ -39,6 +39,7 @@ export {
39
40
// Capture local references to native APIs, in case a polyfill overrides them.
41
const perf = window.performance;
42
+const setTimeout = window.setTimeout;
43
44
// Use experimental Chrome Scheduler postTask API.
45
const scheduler = global.scheduler;
@@ -112,7 +113,7 @@ export function unstable_scheduleCallback<T>(
113
runTask.bind(null, priorityLevel, postTaskPriority, node, callback),
114
postTaskOptions,
115
)
115
- .catch(handlePostTaskError);
116
+ .catch(handleAbortError);
117
118
return node;
119
}
@@ -150,30 +151,28 @@ function runTask<T>(
151
),
152
continuationOptions,
153
)
153
- .catch(handlePostTaskError);
154
+ .catch(handleAbortError);
155
}
156
+ } catch (error) {
157
+ // We're inside a `postTask` promise. If we don't handle this error, then it
158
+ // will trigger an "Unhandled promise rejection" error. We don't want that,
159
+ // but we do want the default error reporting behavior that normal
160
+ // (non-Promise) tasks get for unhandled errors.
161
+ //
162
+ // So we'll re-throw the error inside a regular browser task.
163
+ setTimeout(() => {
164
+ throw error;
165
+ });
166
} finally {
167
currentPriorityLevel_DEPRECATED = NormalPriority;
168
}
169
}
170
160
-function handlePostTaskError(error) {
161
- // This error is either a user error thrown by a callback, or an AbortError
162
- // as a result of a cancellation.
163
- //
164
- // User errors trigger a global `error` event even if we don't rethrow them.
165
- // In fact, if we do rethrow them, they'll get reported to the console twice.
166
- // I'm not entirely sure the current `postTask` spec makes sense here. If I
167
- // catch a `postTask` call, it shouldn't trigger a global error.
168
- //
171
+function handleAbortError(error) {
172
// Abort errors are an implementation detail. We don't expose the
173
// TaskController to the user, nor do we expose the promise that is returned
171
- // from `postTask`. So we shouldn't rethrow those, either, since there's no
172
- // way to handle them. (If we did return the promise to the user, then it
173
- // should be up to them to handle the AbortError.)
174
- //
175
- // In either case, we can suppress the error, barring changes to the spec
176
- // or the Scheduler API.
174
+ // from `postTask`. So we should suppress them, since there's no way for the
175
+ // user to handle them.
176
}
177
178
export function unstable_cancelCallback(node: CallbackNode) {
packages/scheduler/src/__tests__/SchedulerPostTask-test.js
+14
-16
@@ -70,6 +70,15 @@ describe('SchedulerPostTask', () => {
70
},
71
};
72
73
+ // Note: setTimeout is used to report errors and nothing else.
74
+ window.setTimeout = cb => {
75
+ try {
76
+ cb();
77
+ } catch (error) {
78
+ runtime.log(`Error: ${error.message}`);
79
+ }
80
+ };
81
+
82
// Mock browser scheduler.
83
const scheduler = {};
84
global.scheduler = scheduler;
@@ -116,16 +125,10 @@ describe('SchedulerPostTask', () => {
125
// delete the continuation task.
126
const prevTaskQueue = taskQueue;
127
taskQueue = new Map();
119
- for (const [, {id, callback, resolve, reject}] of prevTaskQueue) {
120
- try {
121
- log(`Task ${id} Fired`);
122
- callback(false);
123
- resolve();
124
- } catch (error) {
125
- log(`Task ${id} errored [${error.message}]`);
126
- reject(error);
127
- continue;
128
- }
128
+ for (const [, {id, callback, resolve}] of prevTaskQueue) {
129
+ log(`Task ${id} Fired`);
130
+ callback(false);
131
+ resolve();
132
}
133
}
134
function log(val) {
@@ -219,12 +222,7 @@ describe('SchedulerPostTask', () => {
222
'Post Task 1 [user-visible]',
223
]);
224
runtime.flushTasks();
222
- runtime.assertLog([
223
- 'Task 0 Fired',
224
- 'Task 0 errored [Oops!]',
225
- 'Task 1 Fired',
226
- 'Yay',
227
- ]);
225
+ runtime.assertLog(['Task 0 Fired', 'Error: Oops!', 'Task 1 Fired', 'Yay']);
226
});
227
228
it('schedule new task after queue has emptied', () => {