Skip to content

Commit

Permalink
Fix case of scheduled callbacks which schedule other callbacks
Browse files Browse the repository at this point in the history 
**what is the change?:**
We added support for serial scheduled callbacks to schedule more
callbacks, and maintained the order of first-in first-called.

**why make this change?:**
This is sort of a corner case, but it's totally possible and we do
something similar in React.

We wouldn't do this with serial callbacks, but with deferred callbacks
we do a pattern like this:
```
   +                                             +
   |   +--------------------+ +----------------+ | +--------------------------------+ +-------------------------+
   |   |                    | |                | | |                                | |                         |
   |   | main thread blocked| |callback A runs | | | main thread blocked again      | |callback A runs again, finishes
   |   +--------------------+ +-----+----------+ | +--------------------------------+ ++------------------------+
   v                                ^            v                                     ^
schedule +------------------------->+            no time left!+----------------------->+
callbackA                                        reschedule   |
                                                 callbackA    +
                                                 to do more
                                                 work later.

```

**test plan:**
Wrote some fun new tests and ran them~
Also ran existing React unit tests. As of this PR they are still
directly using this module.
  • Loading branch information
@flarnie
flarnie committed Apr 25, 2018
1 parent 48791d1 commit aa53e84
Show file tree
Hide file tree
Showing 2 changed files with 223 additions and 44 deletions.
85 changes: 64 additions & 21 deletions packages/react-scheduler/src/ReactScheduler.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
* - Better test coverage
* - Mock out the react-scheduler module, not the browser APIs, in renderer
* tests
* - Add headless browser test of react-scheduler
* - Add fixture test of react-scheduler
* - Better docblock
* - Polish documentation, API
*/
Expand Down Expand Up @@ -90,9 +90,12 @@ if (!ExecutionEnvironment.canUseDOM) {
} else {
// Always polyfill requestIdleCallback and cancelIdleCallback

let scheduledRICCallback = null;
let scheduledCallback = null;
let isIdleScheduled = false;
let timeoutTime = -1;
let isCurrentlyRunningCallback = false;
// We may need to keep a queue of pending callbacks
let pendingCallbacks = [];

let isAnimationFrameScheduled = false;

Expand All @@ -111,12 +114,6 @@ if (!ExecutionEnvironment.canUseDOM) {
},
};

// define a helper for this because they should usually happen together
const resetScheduledCallback = function() {
timeoutTime = -1;
scheduledRICCallback = null;
};

// We use the postMessage trick to defer idle work until after the repaint.
const messageKey =
'__reactIdleCallback$' +
Expand Down Expand Up @@ -154,11 +151,14 @@ if (!ExecutionEnvironment.canUseDOM) {
didTimeout = false;
}

const callback = scheduledRICCallback;
resetScheduledCallback();
const callback = scheduledCallback;
timeoutTime = -1;
scheduledCallback = null;
if (callback !== null) {
frameDeadlineObject.didTimeout = didTimeout;
isCurrentlyRunningCallback = true;
callback(frameDeadlineObject);
isCurrentlyRunningCallback = false;
}
};
// Assumes that we have addEventListener in this environment. Might need
Expand Down Expand Up @@ -200,35 +200,78 @@ if (!ExecutionEnvironment.canUseDOM) {
callback: (deadline: Deadline) => void,
options?: {timeout: number},
): number {
if (scheduledRICCallback !== null) {
// Handling multiple callbacks:
// For now we implement the behavior expected when the callbacks are
// serial updates, such that each update relies on the previous ones
// having been called before it runs.
frameDeadlineObject.didTimeout =
(timeoutTime !== -1) && (timeoutTime <= now());
scheduledRICCallback(frameDeadlineObject);
// Handling multiple callbacks:
// For now we implement the behavior expected when the callbacks are
// serial updates, such that each update relies on the previous ones
// having been called before it runs.
// So we call anything in the queue before the latest callback

let previousCallback;
let timeoutTimeFromPreviousCallback;
if (scheduledCallback !== null) {
// If we have previous callback, save it and handle it below
timeoutTimeFromPreviousCallback = timeoutTime;
previousCallback = scheduledCallback;
}
scheduledRICCallback = callback;
// Then set up the next callback, and update timeoutTime
scheduledCallback = callback;
if (options != null && typeof options.timeout === 'number') {
timeoutTime = now() + options.timeout;
} else {
timeoutTime = -1;
}
// If we have previousCallback, call it. This may trigger recursion.
if (
previousCallback &&
typeof timeoutTimeFromPreviousCallback === 'number'
) {
const prevCallbackTimeout: number = timeoutTimeFromPreviousCallback;
if (isCurrentlyRunningCallback) {
// we are inside a recursive call to rIC
// add this callback to a pending queue and run after we exit
pendingCallbacks.push({
pendingCallback: previousCallback,
pendingCallbackTimeout: prevCallbackTimeout,
});
} else {
frameDeadlineObject.didTimeout =
timeoutTimeFromPreviousCallback !== -1 &&
timeoutTimeFromPreviousCallback <= now();
isCurrentlyRunningCallback = true;
previousCallback(frameDeadlineObject);
isCurrentlyRunningCallback = false;
while (pendingCallbacks.length) {
// the callback recursively called rIC and new callbacks are pending
const {
pendingCallback,
pendingCallbackTimeout,
} = pendingCallbacks.shift();
// TODO: pull this into helper method
frameDeadlineObject.didTimeout =
pendingCallbackTimeout !== -1 && pendingCallbackTimeout <= now();
isCurrentlyRunningCallback = true;
pendingCallback(frameDeadlineObject);
isCurrentlyRunningCallback = false;
}
}
}

// finally, after clearing previous callbacks, schedule the latest one
if (!isAnimationFrameScheduled) {
// If rAF didn't already schedule one, we need to schedule a frame.
// TODO: If this rAF doesn't materialize because the browser throttles, we
// might want to still have setTimeout trigger rIC as a backup to ensure
// that we keep performing work.
isAnimationFrameScheduled = true;
requestAnimationFrame(animationTick);
return requestAnimationFrame(animationTick);
}
return 0;
};

cIC = function() {
isIdleScheduled = false;
resetScheduledCallback();
scheduledCallback = null;
timeoutTime = -1;
};
}

Expand Down
182 changes: 159 additions & 23 deletions packages/react-scheduler/src/__tests__/ReactScheduler-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,29 +52,165 @@ describe('ReactScheduler', () => {
expect(typeof cb.mock.calls[0][0].timeRemaining()).toBe('number');
});

it('rIC with multiple callbacks flushes previous cb when new one is passed', () => {
const {rIC} = ReactScheduler;
const callbackLog = [];
const callbackA = jest.fn(() => callbackLog.push('A'));
const callbackB = jest.fn(() => callbackLog.push('B'));
rIC(callbackA);
// initially waits to call the callback
expect(callbackLog.length).toBe(0);
// when second callback is passed, flushes first one
rIC(callbackB);
expect(callbackLog.length).toBe(1);
expect(callbackLog[0]).toBe('A');
// after a delay, calls the latest callback passed
jest.runAllTimers();
expect(callbackLog.length).toBe(2);
expect(callbackLog[0]).toBe('A');
expect(callbackLog[1]).toBe('B');
// callbackA should not have timed out and should include a timeRemaining method
expect(callbackA.mock.calls[0][0].didTimeout).toBe(false);
expect(typeof callbackA.mock.calls[0][0].timeRemaining()).toBe('number');
// callbackA should not have timed out and should include a timeRemaining method
expect(callbackB.mock.calls[0][0].didTimeout).toBe(false);
expect(typeof callbackB.mock.calls[0][0].timeRemaining()).toBe('number');
describe('with multiple callbacks', () => {
it('flushes previous cb when new one is passed', () => {
const {rIC} = ReactScheduler;
const callbackLog = [];
const callbackA = jest.fn(() => callbackLog.push('A'));
const callbackB = jest.fn(() => callbackLog.push('B'));
rIC(callbackA);
// initially waits to call the callback
expect(callbackLog.length).toBe(0);
// when second callback is passed, flushes first one
rIC(callbackB);
expect(callbackLog.length).toBe(1);
expect(callbackLog[0]).toBe('A');
// after a delay, calls the latest callback passed
jest.runAllTimers();
expect(callbackLog.length).toBe(2);
expect(callbackLog[0]).toBe('A');
expect(callbackLog[1]).toBe('B');
// callbackA should not have timed out and should include a timeRemaining method
expect(callbackA.mock.calls[0][0].didTimeout).toBe(false);
expect(typeof callbackA.mock.calls[0][0].timeRemaining()).toBe('number');
// callbackA should not have timed out and should include a timeRemaining method
expect(callbackB.mock.calls[0][0].didTimeout).toBe(false);
expect(typeof callbackB.mock.calls[0][0].timeRemaining()).toBe('number');
});

it('schedules callbacks in correct order when a callback uses rIC before its own logic', () => {
const {rIC} = ReactScheduler;
const callbackLog = [];
const callbackA = jest.fn(() => {
callbackLog.push('A');
rIC(callbackC);
});
const callbackB = jest.fn(() => {
callbackLog.push('B');
});
const callbackC = jest.fn(() => {
callbackLog.push('C');
});

rIC(callbackA);
// initially waits to call the callback
expect(callbackLog.length).toBe(0);
// when second callback is passed, flushes first one
// callbackA scheduled callbackC, which flushes callbackB
rIC(callbackB);
expect(callbackLog.length).toBe(2);
expect(callbackLog[0]).toBe('A');
expect(callbackLog[1]).toBe('B');
// after a delay, calls the latest callback passed
jest.runAllTimers();
expect(callbackLog.length).toBe(3);
expect(callbackLog[0]).toBe('A');
expect(callbackLog[1]).toBe('B');
expect(callbackLog[2]).toBe('C');
});

it('schedules callbacks in correct order when callbacks have many nested rIC calls', () => {
const {rIC} = ReactScheduler;
const callbackLog = [];
const callbackA = jest.fn(() => {
callbackLog.push('A');
rIC(callbackC);
rIC(callbackD);
});
const callbackB = jest.fn(() => {
callbackLog.push('B');
rIC(callbackE);
rIC(callbackF);
});
const callbackC = jest.fn(() => {
callbackLog.push('C');
});
const callbackD = jest.fn(() => {
callbackLog.push('D');
});
const callbackE = jest.fn(() => {
callbackLog.push('E');
});
const callbackF = jest.fn(() => {
callbackLog.push('F');
});

rIC(callbackA);
// initially waits to call the callback
expect(callbackLog.length).toBe(0);
// when second callback is passed, flushes first one
// callbackA scheduled callbackC, which flushes callbackB
rIC(callbackB);
expect(callbackLog.length).toBe(5);
expect(callbackLog[0]).toBe('A');
expect(callbackLog[1]).toBe('B');
expect(callbackLog[2]).toBe('C');
expect(callbackLog[3]).toBe('D');
expect(callbackLog[4]).toBe('E');
// after a delay, calls the latest callback passed
jest.runAllTimers();
expect(callbackLog.length).toBe(6);
expect(callbackLog[5]).toBe('F');
});

it('allows each callback finish running before flushing others', () => {
const {rIC} = ReactScheduler;
const callbackLog = [];
const callbackA = jest.fn(() => {
// rIC should wait to flush any more until this callback finishes
rIC(callbackC);
callbackLog.push('A');
});
const callbackB = jest.fn(() => callbackLog.push('B'));
const callbackC = jest.fn(() => callbackLog.push('C'));

rIC(callbackA);
// initially waits to call the callback
expect(callbackLog.length).toBe(0);
// when second callback is passed, flushes first one
// callbackA scheduled callbackC, which flushes callbackB
rIC(callbackB);
expect(callbackLog.length).toBe(2);
expect(callbackLog[0]).toBe('A');
expect(callbackLog[1]).toBe('B');
// after a delay, calls the latest callback passed
jest.runAllTimers();
expect(callbackLog.length).toBe(3);
expect(callbackLog[0]).toBe('A');
expect(callbackLog[1]).toBe('B');
expect(callbackLog[2]).toBe('C');
});

it('schedules callbacks in correct order when they use rIC to schedule themselves', () => {
const {rIC} = ReactScheduler;
const callbackLog = [];
let callbackAIterations = 0;
const callbackA = jest.fn(() => {
if (callbackAIterations < 1) {
rIC(callbackA);
}
callbackLog.push('A' + callbackAIterations);
callbackAIterations++;
});
const callbackB = jest.fn(() => callbackLog.push('B'));

rIC(callbackA);
// initially waits to call the callback
expect(callbackLog.length).toBe(0);
// when second callback is passed, flushes first one
// callbackA scheduled callbackA again, which flushes callbackB
rIC(callbackB);
expect(callbackLog.length).toBe(2);
expect(callbackLog[0]).toBe('A0');
expect(callbackLog[1]).toBe('B');
// after a delay, calls the latest callback passed
jest.runAllTimers();
expect(callbackLog.length).toBe(3);
expect(callbackLog[0]).toBe('A0');
expect(callbackLog[1]).toBe('B');
expect(callbackLog[2]).toBe('A1');
});
});

// TODO: test cIC and now
});

0 comments on commit aa53e84

Please sign in to comment.