fix(runtime-core/scheduler): only allow watch callbacks to be self-tr…

…iggering

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.

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)
  })
 })

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

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<any> = Promise.resolve()
 let currentFlushPromise: Promise<void> | 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<Job | Function, number>
+type CountMap = Map<SchedulerJob | Function, number>
 
 export function nextTick(fn?: () => void): Promise<void> {
  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
-
- flushPostFlushCbs(seen)
- isFlushing = false
- currentFlushPromise = null
- // some postFlushCb queued jobs!
- // keep flushing until it drains.
- if (queue.length || postFlushCbs.length) {
- flushJobs(seen)
+ } 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)
+ }
  }
 }
 
-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)