Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race condition when calling mutate synchronously #735

Merged
merged 5 commits into from
Nov 5, 2020
Merged

Fix race condition when calling mutate synchronously #735

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bf850e6
fix race condition when calling mutate synchronously
shuding Oct 30, 2020
afef620
fix test
shuding Oct 30, 2020
23dc871
add comment
shuding Oct 30, 2020
6c4df85
fix code reviews
shuding Oct 30, 2020
34d546b
Merge branch 'master' into fix-mutate-race
shuding Oct 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 42 additions & 16 deletions src/use-swr.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Expand Down Expand Up @@ -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))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can keep the previous 'else if' flow here instead of reassign _data here ?

if (_data && typeof _data === 'function') {
    // `_data` is a function, call it passing current cache value
    try {
      data = _data(cache.get(key))
    } catch (err) {
      error = err
    }
  } else if (_data && typeof _data.then === 'function') {
    // `_data` is a promise
    isAsyncMutation = true
    try {
      data = await _data
    } catch (err) {
      error = err
    }
  } else {
    data = _data
  }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can’t. If _data is an async function, calling it without await will get a promise returned, and we have to resolve it again.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On i see now 😅. I think i can add a test for mutate with async function. we don't have one now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@promer94 that would be great, thanks!

} 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) {
Expand All @@ -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
Expand Down Expand Up @@ -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]

Expand Down
59 changes: 49 additions & 10 deletions test/use-swr.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
)
Expand All @@ -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"`)
Expand All @@ -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',
Expand All @@ -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',
Expand All @@ -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"`)
})

Expand Down Expand Up @@ -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', () => {
Expand Down