Skip to content

Interaction tracking ref-counting bug fixes (WIP) #13590

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

Merged
bvaughn merged 4 commits into from
Sep 25, 2018
Merged

Interaction tracking ref-counting bug fixes (WIP) #13590

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
14c37f4
Added new (failing) suspense+interaction tests
Sep 25, 2018
2923bf5
Add new tracing+suspense test harness fixture
Sep 25, 2018
fe9446f
Refactored interaction tracing to fix ref counting bug
Sep 25, 2018
0232cc5
PR feedback: Renamed a var and added an inline comment
Sep 25, 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
Prev Previous commit
PR feedback: Renamed a var and added an inline comment
  • Loading branch information
Brian Vaughn committed Sep 25, 2018
commit 0232cc5b430cd6233cc7f5fef45c22f7252461cb
15 changes: 9 additions & 6 deletions packages/react-reconciler/src/ReactFiberScheduler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ let interruptedBy: Fiber | null = null;
// Do not decrement interaction counts in the event of suspense timeouts.
// This would lead to prematurely calling the interaction-complete hook.
// Instead we wait until the suspended promise has resolved.
let interactionsHaveBeenSuspended: boolean = false;
let nextRenderIncludesTimedOutPlaceholder: boolean = false;

let stashedWorkInProgressProperties;
let replayUnitOfWork;
Expand Down Expand Up @@ -770,13 +770,16 @@ function commitRoot(root: FiberRoot, finishedWork: Fiber): void {
unhandledError = error;
}
} finally {
if (!interactionsHaveBeenSuspended) {
if (!nextRenderIncludesTimedOutPlaceholder) {
// Clear completed interactions from the pending Map.
// Unless the render was suspended or cascading work was scheduled,
// In which case– leave pending interactions until the subsequent render.
const pendingInteractionMap = root.pendingInteractionMap;
pendingInteractionMap.forEach(
(scheduledInteractions, scheduledExpirationTime) => {
// Only decrement the pending interaction count if we're done.
// If there's still work at the current priority,
// That indicates that we are waiting for suspense data.
if (
earliestRemainingTimeAfterCommit === NoWork ||
scheduledExpirationTime < earliestRemainingTimeAfterCommit
Copy link
Collaborator

@acdlite acdlite Sep 25, 2018

Choose a reason for hiding this comment

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

Maybe include a comment explaining why this check works, since it's a bit confusing.

Expand Down Expand Up @@ -1188,7 +1191,7 @@ function renderRoot(
// Reset this flag once we start rendering a new root or at a new priority.
// This might indicate that suspended work has completed.
// If not, the flag will be reset.
interactionsHaveBeenSuspended = false;
nextRenderIncludesTimedOutPlaceholder = false;

// Determine which interactions this batch of work currently includes,
// So that we can accurately attribute time spent working on it,
Expand Down Expand Up @@ -1401,7 +1404,7 @@ function renderRoot(
if (enableSuspense && !isExpired && nextLatestAbsoluteTimeoutMs !== -1) {
// The tree was suspended.
if (enableSchedulerTracing) {
interactionsHaveBeenSuspended = true;
nextRenderIncludesTimedOutPlaceholder = true;
}
const suspendedExpirationTime = expirationTime;
markSuspendedPriorityLevel(root, suspendedExpirationTime);
Expand Down Expand Up @@ -1912,7 +1915,7 @@ function onTimeout(root, finishedWork, suspendedExpirationTime) {
if (enableSchedulerTracing) {
// Don't update pending interaction counts for suspense timeouts,
// Because we know we still need to do more work in this case.
interactionsHaveBeenSuspended = true;
nextRenderIncludesTimedOutPlaceholder = true;
flushRoot(root, suspendedExpirationTime);
} else {
flushRoot(root, suspendedExpirationTime);
Expand Down Expand Up @@ -2486,7 +2489,7 @@ function flushControlled(fn: () => mixed): void {

function captureWillSyncRenderPlaceholder() {
if (enableSchedulerTracing) {
interactionsHaveBeenSuspended = true;
nextRenderIncludesTimedOutPlaceholder = true;
}
}

Expand Down