From 09702e95b9a3f68fc1952ef74555dffa92d50032 Mon Sep 17 00:00:00 2001 From: Evan You Date: Thu, 30 Jul 2020 17:54:05 -0400 Subject: [PATCH] fix(runtime-core/scheduler): only allow watch callbacks to be self-triggering fix #1740 Previous fix for #1727 caused `watchEffect` to also recursively trigger itself on reactive array mutations which implicitly registers array `.length` as dependencies and mutates it at the same time. This fix limits recursive trigger behavior to only `watch()` callbacks since code inside the callback do not register dependencies and mutations are always explicitly intended. --- .../runtime-core/__tests__/scheduler.spec.ts | 44 ++++++++++ packages/runtime-core/src/apiWatch.ts | 8 +- packages/runtime-core/src/scheduler.ts | 87 ++++++++++++------- 3 files changed, 108 insertions(+), 31 deletions(-) diff --git a/packages/runtime-core/__tests__/scheduler.spec.ts b/packages/runtime-core/__tests__/scheduler.spec.ts index cbd349fe..80ba290e 100644 --- a/packages/runtime-core/__tests__/scheduler.spec.ts +++ b/packages/runtime-core/__tests__/scheduler.spec.ts @@ -308,5 +308,49 @@ describe('scheduler', () => { expect( `Unhandled error during execution of scheduler flush` ).toHaveBeenWarned() + + // this one should no longer error + await nextTick() + }) + + test('should prevent self-triggering jobs by default', async () => { + let count = 0 + const job = () => { + if (count < 3) { + count++ + queueJob(job) + } + } + queueJob(job) + await nextTick() + // only runs once - a job cannot queue itself + expect(count).toBe(1) + }) + + test('should allow watcher callbacks to trigger itself', async () => { + // normal job + let count = 0 + const job = () => { + if (count < 3) { + count++ + queueJob(job) + } + } + job.cb = true + queueJob(job) + await nextTick() + expect(count).toBe(3) + + // post cb + const cb = () => { + if (count < 5) { + count++ + queuePostFlushCb(cb) + } + } + cb.cb = true + queuePostFlushCb(cb) + await nextTick() + expect(count).toBe(5) }) }) diff --git a/packages/runtime-core/src/apiWatch.ts b/packages/runtime-core/src/apiWatch.ts index de6f54e9..15c9e3e5 100644 --- a/packages/runtime-core/src/apiWatch.ts +++ b/packages/runtime-core/src/apiWatch.ts @@ -7,7 +7,7 @@ import { ReactiveEffectOptions, isReactive } from '@vue/reactivity' -import { queueJob } from './scheduler' +import { queueJob, SchedulerJob } from './scheduler' import { EMPTY_OBJ, isObject, @@ -232,7 +232,7 @@ function doWatch( } let oldValue = isArray(source) ? [] : INITIAL_WATCHER_VALUE - const job = () => { + const job: SchedulerJob = () => { if (!runner.active) { return } @@ -258,6 +258,10 @@ function doWatch( } } + // important: mark the job as a watcher callback so that scheduler knows it + // it is allowed to self-trigger (#1727) + job.cb = !!cb + let scheduler: (job: () => any) => void if (flush === 'sync') { scheduler = job diff --git a/packages/runtime-core/src/scheduler.ts b/packages/runtime-core/src/scheduler.ts index 07ee84a3..a6933bcc 100644 --- a/packages/runtime-core/src/scheduler.ts +++ b/packages/runtime-core/src/scheduler.ts @@ -1,38 +1,57 @@ import { ErrorCodes, callWithErrorHandling } from './errorHandling' import { isArray } from '@vue/shared' -export interface Job { +export interface SchedulerJob { (): void + /** + * unique job id, only present on raw effects, e.g. component render effect + */ id?: number + /** + * Indicates this is a watch() callback and is allowed to trigger itself. + * A watch callback doesn't track its dependencies so if it triggers itself + * again, it's likely intentional and it is the user's responsibility to + * perform recursive state mutation that eventually stabilizes. + */ + cb?: boolean } -const queue: (Job | null)[] = [] +const queue: (SchedulerJob | null)[] = [] const postFlushCbs: Function[] = [] const resolvedPromise: Promise = Promise.resolve() let currentFlushPromise: Promise | null = null let isFlushing = false let isFlushPending = false -let flushIndex = -1 +let flushIndex = 0 let pendingPostFlushCbs: Function[] | null = null let pendingPostFlushIndex = 0 const RECURSION_LIMIT = 100 -type CountMap = Map +type CountMap = Map export function nextTick(fn?: () => void): Promise { const p = currentFlushPromise || resolvedPromise return fn ? p.then(fn) : p } -export function queueJob(job: Job) { - if (!queue.includes(job, flushIndex + 1)) { +export function queueJob(job: SchedulerJob) { + // the dedupe search uses the startIndex argument of Array.includes() + // by default the search index includes the current job that is being run + // so it cannot recursively trigger itself again. + // if the job is a watch() callback, the search will start with a +1 index to + // allow it recursively trigger itself - it is the user's responsibility to + // ensure it doesn't end up in an infinite loop. + if ( + !queue.length || + !queue.includes(job, job.cb ? flushIndex + 1 : flushIndex) + ) { queue.push(job) queueFlush() } } -export function invalidateJob(job: Job) { +export function invalidateJob(job: SchedulerJob) { const i = queue.indexOf(job) if (i > -1) { queue[i] = null @@ -43,7 +62,12 @@ export function queuePostFlushCb(cb: Function | Function[]) { if (!isArray(cb)) { if ( !pendingPostFlushCbs || - !pendingPostFlushCbs.includes(cb, pendingPostFlushIndex + 1) + !pendingPostFlushCbs.includes( + cb, + (cb as SchedulerJob).cb + ? pendingPostFlushIndex + 1 + : pendingPostFlushIndex + ) ) { postFlushCbs.push(cb) } @@ -85,7 +109,7 @@ export function flushPostFlushCbs(seen?: CountMap) { } } -const getId = (job: Job) => (job.id == null ? Infinity : job.id) +const getId = (job: SchedulerJob) => (job.id == null ? Infinity : job.id) function flushJobs(seen?: CountMap) { isFlushPending = false @@ -105,38 +129,43 @@ function flushJobs(seen?: CountMap) { // during execution of another flushed job. queue.sort((a, b) => getId(a!) - getId(b!)) - for (flushIndex = 0; flushIndex < queue.length; flushIndex++) { - const job = queue[flushIndex] - if (job) { - if (__DEV__) { - checkRecursiveUpdates(seen!, job) + try { + for (flushIndex = 0; flushIndex < queue.length; flushIndex++) { + const job = queue[flushIndex] + if (job) { + if (__DEV__) { + checkRecursiveUpdates(seen!, job) + } + callWithErrorHandling(job, null, ErrorCodes.SCHEDULER) } - callWithErrorHandling(job, null, ErrorCodes.SCHEDULER) } - } - flushIndex = -1 - queue.length = 0 + } finally { + flushIndex = 0 + queue.length = 0 - flushPostFlushCbs(seen) - isFlushing = false - currentFlushPromise = null - // some postFlushCb queued jobs! - // keep flushing until it drains. - if (queue.length || postFlushCbs.length) { - flushJobs(seen) + flushPostFlushCbs(seen) + isFlushing = false + currentFlushPromise = null + // some postFlushCb queued jobs! + // keep flushing until it drains. + if (queue.length || postFlushCbs.length) { + flushJobs(seen) + } } } -function checkRecursiveUpdates(seen: CountMap, fn: Job | Function) { +function checkRecursiveUpdates(seen: CountMap, fn: SchedulerJob | Function) { if (!seen.has(fn)) { seen.set(fn, 1) } else { const count = seen.get(fn)! if (count > RECURSION_LIMIT) { throw new Error( - 'Maximum recursive updates exceeded. ' + - "You may have code that is mutating state in your component's " + - 'render function or updated hook or watcher source function.' + `Maximum recursive updates exceeded. ` + + `This means you have a reactive effect that is mutating its own ` + + `dependencies and thus recursively triggering itself. Possible sources ` + + `include component template, render function, updated hook or ` + + `watcher source function.` ) } else { seen.set(fn, count + 1)