From bf850e6da3603ab67cebc77bc34c5147240bad43 Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Fri, 30 Oct 2020 21:35:26 +0800 Subject: [PATCH 1/4] fix race condition when calling mutate synchronously --- src/use-swr.ts | 48 +++++++++++++++++++++++++++-------- test/use-swr.test.tsx | 58 +++++++++++++++++++++++++++++++++++-------- 2 files changed, 86 insertions(+), 20 deletions(-) diff --git a/src/use-swr.ts b/src/use-swr.ts index ffaa8c8a6..a6fa196e8 100644 --- a/src/use-swr.ts +++ b/src/use-swr.ts @@ -45,6 +45,12 @@ const CACHE_REVALIDATORS = {} const MUTATION_TS = {} const MUTATION_END_TS = {} +// generate strictly increasing timestamps +let 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 isMutationAsync = 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 + isMutationAsync = true try { data = await _data } catch (err) { @@ -153,7 +163,8 @@ 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 + // check if other mutations have occurred since we've started this mutation + // if so we don't update cache or broadcast change, just return the data if ( beforeMutationTs !== MUTATION_TS[key] || beforeConcurrentPromisesTs !== CONCURRENT_PROMISES_TS[key] @@ -163,13 +174,30 @@ const mutate: mutateInterface = async ( } 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 (!isMutationAsync) { + // let's always broadcast in the next tick + // to dedupe synchronous mutation calls + await 0 + + // we skip broadcasting if there's another mutation + // happened synchronously + if ( + beforeMutationTs !== MUTATION_TS[key] || + beforeConcurrentPromisesTs !== CONCURRENT_PROMISES_TS[key] + ) { + if (error) throw error + return data + } + } // enter the revalidation stage // update existing SWR Hooks' state @@ -385,7 +413,7 @@ function useSWR( CONCURRENT_PROMISES[key] = fn(key) } - CONCURRENT_PROMISES_TS[key] = startAt = Date.now() + CONCURRENT_PROMISES_TS[key] = startAt = now() newData = await CONCURRENT_PROMISES[key] diff --git a/test/use-swr.test.tsx b/test/use-swr.test.tsx index 38cb74710..7f0fd344d 100644 --- a/test/use-swr.test.tsx +++ b/test/use-swr.test.tsx @@ -1499,7 +1499,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"` ) @@ -1543,9 +1544,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', @@ -1560,12 +1566,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', @@ -1580,15 +1588,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"`) }) @@ -1706,6 +1711,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() + + await act(() => new Promise(res => setTimeout(res, 50))) + + expect(mutationRecivedValues).toEqual([0, 1]) + expect(renderRecivedValues).toEqual([undefined, 0, 2]) + }) }) describe('useSWR - context configs', () => { From afef6202bb4928d875b43c9668221935dac4e7a5 Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Fri, 30 Oct 2020 22:12:00 +0800 Subject: [PATCH 2/4] fix test --- test/use-swr.test.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/test/use-swr.test.tsx b/test/use-swr.test.tsx index 7f0fd344d..2359c587a 100644 --- a/test/use-swr.test.tsx +++ b/test/use-swr.test.tsx @@ -1523,6 +1523,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"`) From 23dc87144a47637a425070c3acc71a4c4908b28f Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Fri, 30 Oct 2020 22:13:05 +0800 Subject: [PATCH 3/4] add comment --- src/use-swr.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/use-swr.ts b/src/use-swr.ts index a6fa196e8..811f4b2ca 100644 --- a/src/use-swr.ts +++ b/src/use-swr.ts @@ -186,6 +186,7 @@ const mutate: mutateInterface = async ( if (!isMutationAsync) { // 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 From 6c4df857d3fe32c3d724a1fd2a1d943249a6d802 Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Sat, 31 Oct 2020 00:17:08 +0800 Subject: [PATCH 4/4] fix code reviews --- src/use-swr.ts | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/src/use-swr.ts b/src/use-swr.ts index 811f4b2ca..a36a58ca1 100644 --- a/src/use-swr.ts +++ b/src/use-swr.ts @@ -46,7 +46,7 @@ const MUTATION_TS = {} const MUTATION_END_TS = {} // generate strictly increasing timestamps -let now = (() => { +const now = (() => { let ts = 0 return () => ts++ })() @@ -140,7 +140,7 @@ const mutate: mutateInterface = async ( const beforeConcurrentPromisesTs = CONCURRENT_PROMISES_TS[key] let data, error - let isMutationAsync = false + let isAsyncMutation = false if (_data && typeof _data === 'function') { // `_data` is a function, call it passing current cache value @@ -153,7 +153,7 @@ const mutate: mutateInterface = async ( if (_data && typeof _data.then === 'function') { // `_data` is a promise - isMutationAsync = true + isAsyncMutation = true try { data = await _data } catch (err) { @@ -163,16 +163,20 @@ const mutate: mutateInterface = async ( data = _data } - // check if other mutations have occurred since we've started this mutation - // if so we don't update cache or broadcast change, just return the data - 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 cache.set(key, data) @@ -183,21 +187,14 @@ const mutate: mutateInterface = async ( // reset the timestamp to mark the mutation has ended MUTATION_END_TS[key] = now() - 1 - if (!isMutationAsync) { + 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 ( - beforeMutationTs !== MUTATION_TS[key] || - beforeConcurrentPromisesTs !== CONCURRENT_PROMISES_TS[key] - ) { - if (error) throw error - return data - } + // we skip broadcasting if there's another mutation happened synchronously + if (shouldAbort()) return data } // enter the revalidation stage