Skip to content

Commit 07deb2c

Browse files
VarixoVarixo
committed
fix: skip apply journal during vnode diff
1 parent da59cf0 commit 07deb2c

File tree

3 files changed

+210
-12
lines changed

3 files changed

+210
-12
lines changed

‎packages/qwik/src/core/shared/scheduler-journal-block.unit.tsx‎

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
2+
import { $ } from '@qwik.dev/core';
3+
import { delay } from './utils/promises';
4+
import { Chore, createScheduler } from './scheduler';
5+
import { createDocument } from '@qwik.dev/core/testing';
6+
import { QContainerAttr } from './utils/markers';
7+
import { getDomContainer } from '../client/dom-container';
8+
import { ChoreArray } from '../client/chore-array';
9+
import { ChoreType } from './util-chore-type';
10+
import type { HostElement } from './types';
11+
import { _jsxSorted } from '../internal';
12+
import type { ElementVNode, VirtualVNode } from '../client/vnode-impl';
13+
import {
14+
vnode_insertBefore,
15+
vnode_locate,
16+
vnode_newUnMaterializedElement,
17+
vnode_newVirtual,
18+
} from '../client/vnode';
19+
20+
vi.mock('../client/vnode-diff', () => ({
21+
vnode_diff: vi.fn().mockImplementation(async () => {
22+
await delay(100);
23+
}),
24+
}));
25+
26+
describe('should block journal flush during node-diff and component runs', () => {
27+
let scheduler: ReturnType<typeof createScheduler> = null!;
28+
let document: ReturnType<typeof createDocument> = null!;
29+
let vBody: ElementVNode = null!;
30+
let vA: ElementVNode = null!;
31+
let vAHost: VirtualVNode = null!;
32+
let choreQueue: ChoreArray;
33+
let blockedChores: Set<Chore>;
34+
let runningChores: Set<Chore>;
35+
36+
async function waitForDrain() {
37+
await scheduler(ChoreType.WAIT_FOR_QUEUE).$returnValue$;
38+
}
39+
40+
beforeEach(() => {
41+
(globalThis as any).testLog = [];
42+
document = createDocument();
43+
document.body.setAttribute(QContainerAttr, 'paused');
44+
const container = getDomContainer(document.body);
45+
container.handleError = vi.fn();
46+
choreQueue = new ChoreArray();
47+
blockedChores = new Set();
48+
runningChores = new Set();
49+
scheduler = createScheduler(
50+
container,
51+
() => testLog.push('journalFlush'),
52+
choreQueue,
53+
blockedChores,
54+
runningChores
55+
);
56+
document.body.innerHTML = '<a :></a><b :></b>';
57+
vBody = vnode_newUnMaterializedElement(document.body);
58+
vA = vnode_locate(vBody, document.querySelector('a') as Element) as ElementVNode;
59+
vAHost = vnode_newVirtual();
60+
vAHost.setProp('q:id', 'A');
61+
vnode_insertBefore([], vA, vAHost, null);
62+
vi.useFakeTimers();
63+
});
64+
65+
afterEach(async () => {
66+
vi.useRealTimers();
67+
await waitForDrain();
68+
});
69+
70+
it('should block journal flush when NODE_DIFF is scheduled and executing', async () => {
71+
scheduler(
72+
ChoreType.NODE_DIFF,
73+
vAHost as HostElement,
74+
vAHost as HostElement,
75+
_jsxSorted('div', {}, null, null, 0, null)
76+
);
77+
78+
expect(choreQueue.length).toBe(1);
79+
vi.advanceTimersToNextTimer();
80+
expect(runningChores.size).toBe(1);
81+
await vi.advanceTimersByTimeAsync(20);
82+
// no journal flush even if time elapsed, because NODE_DIFF is running
83+
expect(testLog).toEqual([]);
84+
// finish VNODE_DIFF
85+
await vi.advanceTimersByTimeAsync(80);
86+
// no running chores
87+
expect(runningChores.size).toBe(0);
88+
// journal flush should have happened
89+
expect(testLog).toEqual(['journalFlush']);
90+
});
91+
92+
it('should block journal flush when NODE_DIFF and COMPONENT is scheduled and executing', async () => {
93+
scheduler(
94+
ChoreType.NODE_DIFF,
95+
vAHost as HostElement,
96+
vAHost as HostElement,
97+
_jsxSorted('div', {}, null, null, 0, null)
98+
);
99+
100+
expect(choreQueue.length).toBe(1);
101+
vi.advanceTimersToNextTimer();
102+
expect(runningChores.size).toBe(1);
103+
await vi.advanceTimersByTimeAsync(80);
104+
// no journal flush even if time elapsed, because NODE_DIFF is running
105+
expect(testLog).toEqual([]);
106+
// schedule component chore while NODE_DIFF is running
107+
scheduler(
108+
ChoreType.COMPONENT,
109+
vAHost as HostElement,
110+
$(async () => {
111+
await delay(50);
112+
}) as any,
113+
null
114+
);
115+
// finish VNODE_DIFF
116+
await vi.advanceTimersByTimeAsync(20);
117+
// no running chores
118+
expect(runningChores.size).toBe(1);
119+
// still no journal flush because COMPONENT is running
120+
expect(testLog).toEqual([]);
121+
await vi.advanceTimersByTimeAsync(20);
122+
// still no journal flush because COMPONENT is running
123+
expect(testLog).toEqual([]);
124+
// finish COMPONENT + next VNODE_DIFF
125+
await vi.advanceTimersByTimeAsync(110);
126+
expect(runningChores.size).toBe(0);
127+
expect(testLog).toEqual(['journalFlush']);
128+
});
129+
});

‎packages/qwik/src/core/shared/scheduler.ts‎

Lines changed: 80 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,8 @@ export const createScheduler = (
194194
let currentTime = performance.now();
195195
const nextTick = createNextTick(drainChoreQueue);
196196
let flushTimerId: number | null = null;
197+
let blockingChoresCount = 0;
198+
let currentChore: Chore | null = null;
197199

198200
function drainInNextTick() {
199201
if (!drainScheduled) {
@@ -275,6 +277,7 @@ export const createScheduler = (
275277
if (isTask) {
276278
(hostOrTask as Task).$flags$ |= TaskFlags.DIRTY;
277279
}
280+
278281
const chore: Chore<T> = {
279282
$type$: type,
280283
$idx$: isTask
@@ -345,6 +348,10 @@ This is often caused by modifying a signal in an already rendered component duri
345348
logWarn(warningMessage);
346349
DEBUG &&
347350
debugTrace('schedule.SKIPPED host is not updatable', chore, choreQueue, blockedChores);
351+
// Decrement counter if this was a blocking chore that we're skipping
352+
if (isRenderBlocking(type)) {
353+
blockingChoresCount--;
354+
}
348355
return chore;
349356
}
350357
}
@@ -373,8 +380,15 @@ This is often caused by modifying a signal in an already rendered component duri
373380
}
374381
}
375382

376-
addChore(chore, choreQueue);
377-
DEBUG && debugTrace('schedule', chore, choreQueue, blockedChores);
383+
addChoreAndIncrementBlockingCounter(chore, choreQueue);
384+
385+
DEBUG &&
386+
debugTrace(
387+
isRenderBlocking(type) ? `schedule (blocking ${blockingChoresCount})` : 'schedule',
388+
chore,
389+
choreQueue,
390+
blockedChores
391+
);
378392

379393
const runImmediately = (isServer && type === ChoreType.COMPONENT) || type === ChoreType.RUN_QRL;
380394

@@ -432,6 +446,16 @@ This is often caused by modifying a signal in an already rendered component duri
432446
}
433447

434448
function applyJournalFlush() {
449+
if (blockingChoresCount > 0) {
450+
DEBUG &&
451+
debugTrace(
452+
`journalFlush.BLOCKED (${blockingChoresCount} blocking chores)`,
453+
null,
454+
choreQueue,
455+
blockedChores
456+
);
457+
return;
458+
}
435459
if (!isJournalFlushRunning) {
436460
// prevent multiple journal flushes from running at the same time
437461
isJournalFlushRunning = true;
@@ -509,7 +533,7 @@ This is often caused by modifying a signal in an already rendered component duri
509533
if (vnode_isVNode(blockedChore.$host$)) {
510534
blockedChore.$host$.blockedChores?.delete(blockedChore);
511535
}
512-
addChore(blockedChore, choreQueue);
536+
addChoreAndIncrementBlockingCounter(blockedChore, choreQueue);
513537
DEBUG && debugTrace('schedule.UNBLOCKED', blockedChore, choreQueue, blockedChores);
514538
blockedChoresScheduled = true;
515539
}
@@ -521,13 +545,12 @@ This is often caused by modifying a signal in an already rendered component duri
521545
}
522546
};
523547

524-
let currentChore: Chore | null = null;
525-
526548
try {
527549
while (choreQueue.length) {
528550
currentTime = performance.now();
529551
const chore = (currentChore = choreQueue.shift()!);
530552
if (chore.$state$ !== ChoreState.NONE) {
553+
// Chore was already processed, counter already decremented in finishChore/handleError
531554
continue;
532555
}
533556

@@ -541,6 +564,10 @@ This is often caused by modifying a signal in an already rendered component duri
541564
if (vnode_isVNode(chore.$host$)) {
542565
chore.$host$.chores?.delete(chore);
543566
}
567+
// Decrement counter if this was a blocking chore that we're skipping
568+
if (isRenderBlocking(chore.$type$)) {
569+
blockingChoresCount--;
570+
}
544571
continue;
545572
}
546573

@@ -618,13 +645,40 @@ This is often caused by modifying a signal in an already rendered component duri
618645
if (vnode_isVNode(chore.$host$)) {
619646
chore.$host$.chores?.delete(chore);
620647
}
621-
DEBUG && debugTrace('execute.DONE', chore, choreQueue, blockedChores);
648+
649+
// Decrement blocking counter if this chore was blocking journal flush
650+
if (isRenderBlocking(chore.$type$)) {
651+
blockingChoresCount--;
652+
DEBUG &&
653+
debugTrace(
654+
`execute.DONE (blocking ${blockingChoresCount})`,
655+
chore,
656+
choreQueue,
657+
blockedChores
658+
);
659+
} else {
660+
DEBUG && debugTrace('execute.DONE', chore, choreQueue, blockedChores);
661+
}
622662
}
623663

624664
function handleError(chore: Chore, e: any) {
625665
chore.$endTime$ = performance.now();
626666
chore.$state$ = ChoreState.FAILED;
627-
DEBUG && debugTrace('execute.ERROR', chore, choreQueue, blockedChores);
667+
668+
// Decrement blocking counter if this chore was blocking journal flush
669+
if (isRenderBlocking(chore.$type$)) {
670+
blockingChoresCount--;
671+
DEBUG &&
672+
debugTrace(
673+
`execute.ERROR (blocking ${blockingChoresCount})`,
674+
chore,
675+
choreQueue,
676+
blockedChores
677+
);
678+
} else {
679+
DEBUG && debugTrace('execute.ERROR', chore, choreQueue, blockedChores);
680+
}
681+
628682
// If we used the result as promise, this won't exist
629683
chore.$reject$?.(e);
630684
container.handleError(e, chore.$host$);
@@ -814,8 +868,25 @@ This is often caused by modifying a signal in an already rendered component duri
814868
}
815869
return null;
816870
}
871+
872+
function addChoreAndIncrementBlockingCounter(chore: Chore, choreArray: ChoreArray) {
873+
if (addChore(chore, choreArray)) {
874+
blockingChoresCount++;
875+
}
876+
}
817877
};
818878

879+
export function addChore(chore: Chore, choreArray: ChoreArray): boolean {
880+
const idx = choreArray.add(chore);
881+
if (idx < 0) {
882+
if (vnode_isVNode(chore.$host$)) {
883+
(chore.$host$.chores ||= new ChoreArray()).add(chore);
884+
}
885+
return isRenderBlocking(chore.$type$);
886+
}
887+
return false;
888+
}
889+
819890
function vNodeAlreadyDeleted(chore: Chore): boolean {
820891
return !!(chore.$host$ && vnode_isVNode(chore.$host$) && chore.$host$.flags & VNodeFlags.Deleted);
821892
}
@@ -839,11 +910,8 @@ export function addBlockedChore(
839910
}
840911
}
841912

842-
export function addChore(chore: Chore, choreArray: ChoreArray) {
843-
const idx = choreArray.add(chore);
844-
if (idx < 0 && vnode_isVNode(chore.$host$)) {
845-
(chore.$host$.chores ||= new ChoreArray()).add(chore);
846-
}
913+
function isRenderBlocking(type: ChoreType): boolean {
914+
return type === ChoreType.NODE_DIFF || type === ChoreType.COMPONENT;
847915
}
848916

849917
function choreTypeToName(type: ChoreType): string {

‎packages/qwik/src/core/shared/scheduler.unit.tsx‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1216,6 +1216,7 @@ describe('scheduler', () => {
12161216
expect(chore.$type$).not.toBe(ChoreType.QRL_RESOLVE);
12171217
}
12181218

1219+
await waitForDrain();
12191220
nextTickSpy.mockRestore();
12201221
});
12211222
});

0 commit comments

Comments
 (0)