Fix race condition when calling mutate synchronously (#735)

* fix race condition when calling mutate synchronously

* fix test

* add comment

* fix code reviews

Author: shuding

src/use-swr.ts

@@ -45,6 +45,12 @@ const CACHE_REVALIDATORS = {}
 const MUTATION_TS = {}
 const MUTATION_END_TS = {}
 
+// generate strictly increasing timestamps
+const now = (() => {
+  let ts = 0
+  return () => ts++
+})()
+
 // setup DOM events listeners for `focus` and `reconnect` actions
 if (!IS_SERVER && window.addEventListener) {
   const revalidate = revalidators => {
@@ -122,28 +128,32 @@ const mutate: mutateInterface = async (
   const [key, , keyErr] = cache.serializeKey(_key)
   if (!key) return
 
-  // if there is no new data, call revalidate against the key
+  // if there is no new data to update, let's just revalidate the key
   if (typeof _data === 'undefined') return trigger(_key, shouldRevalidate)
 
-  // update timestamps
-  MUTATION_TS[key] = Date.now() - 1
+  // update global timestamps
+  MUTATION_TS[key] = now() - 1
   MUTATION_END_TS[key] = 0
 
-  // keep track of timestamps before await asynchronously
+  // track timestamps before await asynchronously
   const beforeMutationTs = MUTATION_TS[key]
   const beforeConcurrentPromisesTs = CONCURRENT_PROMISES_TS[key]
 
   let data, error
+  let isAsyncMutation = false
 
   if (_data && typeof _data === 'function') {
     // `_data` is a function, call it passing current cache value
     try {
-      data = await _data(cache.get(key))
+      _data = _data(cache.get(key))
     } catch (err) {
       error = err
     }
-  } else if (_data && typeof _data.then === 'function') {
+  }
+
+  if (_data && typeof _data.then === 'function') {
     // `_data` is a promise
+    isAsyncMutation = true
     try {
       data = await _data
     } catch (err) {
@@ -153,23 +163,39 @@ const mutate: mutateInterface = async (
     data = _data
   }
 
-  // check if other mutations have occurred since we've started awaiting, if so then do not persist this change
-  if (
-    beforeMutationTs !== MUTATION_TS[key] ||
-    beforeConcurrentPromisesTs !== CONCURRENT_PROMISES_TS[key]
-  ) {
-    if (error) throw error
-    return data
+  const shouldAbort = (): boolean | void => {
+    // check if other mutations have occurred since we've started this mutation
+    if (
+      beforeMutationTs !== MUTATION_TS[key] ||
+      beforeConcurrentPromisesTs !== CONCURRENT_PROMISES_TS[key]
+    ) {
+      if (error) throw error
+      return true
+    }
   }
 
+  // if there's a race we don't update cache or broadcast change, just return the data
+  if (shouldAbort()) return data
+
   if (typeof data !== 'undefined') {
-    // update cached data, avoid notifying from the cache
+    // update cached data
     cache.set(key, data)
   }
+  // always update or reset the error
   cache.set(keyErr, error)
 
   // reset the timestamp to mark the mutation has ended
-  MUTATION_END_TS[key] = Date.now() - 1
+  MUTATION_END_TS[key] = now() - 1
+
+  if (!isAsyncMutation) {
+    // let's always broadcast in the next tick
+    // to dedupe synchronous mutation calls
+    // check out https://github.com/vercel/swr/pull/735 for more details
+    await 0
+
+    // we skip broadcasting if there's another mutation happened synchronously
+    if (shouldAbort()) return data
+  }
 
   // enter the revalidation stage
   // update existing SWR Hooks' state
@@ -385,7 +411,7 @@ function useSWR<Data = any, Error = any>(
             CONCURRENT_PROMISES[key] = fn(key)
           }
 
-          CONCURRENT_PROMISES_TS[key] = startAt = Date.now()
+          CONCURRENT_PROMISES_TS[key] = startAt = now()
 
           newData = await CONCURRENT_PROMISES[key]
 

test/use-swr.test.tsx

@@ -1524,7 +1524,8 @@ describe('useSWR - local mutation', () => {
     )
     // call bound mutate
     fireEvent.click(container.firstElementChild)
-    // expect new updated value
+    // expect new updated value (after a tick)
+    await 0
     expect(container.firstChild.textContent).toMatchInlineSnapshot(
       `"data: mutated"`
     )
@@ -1547,6 +1548,7 @@ describe('useSWR - local mutation', () => {
     expect(container.textContent).toMatchInlineSnapshot(`"1"`) // directly from cache
     await act(() => new Promise(res => setTimeout(res, 150))) // still suspending
     mutate('mutate-2', 3) // set it to 3. this will drop the ongoing request
+    await 0
     expect(container.textContent).toMatchInlineSnapshot(`"3"`)
     await act(() => new Promise(res => setTimeout(res, 100)))
     expect(container.textContent).toMatchInlineSnapshot(`"3"`)
@@ -1568,9 +1570,14 @@ describe('useSWR - local mutation', () => {
     await act(() => new Promise(res => setTimeout(res, 250)))
     expect(container.textContent).toMatchInlineSnapshot(`"off"`) // Initial state
 
+    mutate('mutate-3', 'on', false)
+
+    // Validate local state is now "on"
+    await 0
+    expect(container.textContent).toMatchInlineSnapshot(`"on"`)
+
     // Simulate toggling "on"
     await act(async () => {
-      mutate('mutate-3', 'on', false)
       expect(
         mutate(
           'mutate-3',
@@ -1585,12 +1592,14 @@ describe('useSWR - local mutation', () => {
       ).resolves.toBe('on')
     })
 
-    // Validate local state is now "on"
-    expect(container.textContent).toMatchInlineSnapshot(`"on"`)
+    mutate('mutate-3', 'off', false)
+
+    // Validate local state is now "off"
+    await 0
+    expect(container.textContent).toMatchInlineSnapshot(`"off"`)
 
     // Simulate toggling "off"
     await act(async () => {
-      mutate('mutate-3', 'off', false)
       expect(
         mutate(
           'mutate-3',
@@ -1605,15 +1614,12 @@ describe('useSWR - local mutation', () => {
       ).resolves.toBe('off')
     })
 
-    // Validate local state is now "off"
-    expect(container.textContent).toMatchInlineSnapshot(`"off"`)
-
     // Wait for toggling "on" promise to resolve, but the "on" mutation is cancelled
-    await act(() => new Promise(res => setTimeout(res, 200)))
+    await act(() => new Promise(res => setTimeout(res, 210)))
     expect(container.textContent).toMatchInlineSnapshot(`"off"`)
 
     // Wait for toggling "off" promise to resolve
-    await act(() => new Promise(res => setTimeout(res, 200)))
+    await act(() => new Promise(res => setTimeout(res, 210)))
     expect(container.textContent).toMatchInlineSnapshot(`"off"`)
   })
 
@@ -1731,6 +1737,39 @@ describe('useSWR - local mutation', () => {
       expect(ref).toEqual(refs[0])
     }
   })
+
+  it('should dedupe synchronous mutations', async () => {
+    const mutationRecivedValues = []
+    const renderRecivedValues = []
+
+    function Component() {
+      const { data, mutate: boundMutate } = useSWR('mutate-6', () => 0)
+
+      useEffect(() => {
+        setTimeout(() => {
+          // let's mutate twice, synchronously
+          boundMutate(v => {
+            mutationRecivedValues.push(v) // should be 0
+            return 1
+          }, false)
+          boundMutate(v => {
+            mutationRecivedValues.push(v) // should be 1
+            return 2
+          }, false)
+        }, 1)
+      }, [])
+
+      renderRecivedValues.push(data) // should be 0 -> 2, never render 1 in between
+      return null
+    }
+
+    render(<Component />)
+
+    await act(() => new Promise(res => setTimeout(res, 50)))
+
+    expect(mutationRecivedValues).toEqual([0, 1])
+    expect(renderRecivedValues).toEqual([undefined, 0, 2])
+  })
 })
 
 describe('useSWR - context configs', () => {