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.

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