Skip to content

fix: cleanup pending promises on unmount #3125

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+58 −3
Open

fix: cleanup pending promises on unmount #3125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 9 additions & 3 deletions src/vanilla/internals.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,12 +241,13 @@ const setAtomStateValueOrPromise = (
const atomState = ensureAtomState(atom)
const hasPrevValue = 'v' in atomState
const prevValue = atomState.v
if (isPromiseLike(valueOrPromise)) {
atomState.v = valueOrPromise
if (isPromiseLike(atomState.v)) {
for (const a of atomState.d.keys()) {
addPendingPromiseToDependency(atom, valueOrPromise, ensureAtomState(a))
const aState = ensureAtomState(a)
addPendingPromiseToDependency(atom, atomState.v, aState)
}
}
atomState.v = valueOrPromise
delete atomState.e
if (!hasPrevValue || !Object.is(prevValue, atomState.v)) {
++atomState.n
Expand Down Expand Up @@ -834,6 +835,11 @@ const buildStore = (
mounted = undefined
mountedMap.delete(atom)
storeHooks.u?.(atom)
// clean up pending promises
Copy link
Contributor

@a10nik a10nik Aug 14, 2025

Choose a reason for hiding this comment

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

See #3124 (reply in thread)

Still it seems to work better now than before so I would not mind sticking with this partial solution. Can start a new discussion to cover more cases

Copy link
Contributor

@a10nik a10nik Aug 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Having second thoughts about it now:

I've tried to think of possible reasons behind that recomputeInvalidatedAtoms -> getMountedOrPendingDependents combo: essentially, why we are making an unmounted atom with a pending promise react eagerly to its dependencies' changes.

Came up with this Suspense case:

  • An async atom is fetched without mounting during the first Suspense render, it suspends with a promise
  • Its sync dep changes during the pending phase thus making the initial promise stale
  • In this case we would like it to trigger its abort signal and recompute a new value ASAP
  • However, if the unmounted-but-pending atom wasn't eagerly recomputed, it wouldn't be until the next render that it's fetched again. In most cases the render only happens when the initial, now-stale promise is resolved. So the atom won't realize it's stale until then and won't trigger another getter: so the loading will take longer and the abort signal won't fire.

This particular case will not be affected by the PR. Although this similar one will:

  • An async atom is fetched and successfully mounted after its initial promise resolution in ComponentA
  • Then a dep is changed, it start recomputing a new promise
  • ComponentB starts rendering and suspends with that promise
  • While it's still pending, ComponentA is unmounted
  • Then, while it's still pending, the dep changes again
  • If we stopped the eagerness on unmount: ComponentB is now suspended on a stale promise with no way of knowing it's stale until it's finished

See https://stackblitz.com/edit/vitejs-vite-qkbef5vo?file=src%2FApp.tsx:
With original jotai it outputs:


> ⏳ start fetch [dep=0, id=1]
> ✅ resolve fetch [dep=0, id=1]
> mount A
StrictMode> unmount A
StrictMode> mount A
> ⏳ start fetch [dep=1, id=2]
> unmount A
> ⏳ start fetch [dep=2, id=3]
> 🛑 abort fetch [dep=1, id=2]
> ✅ resolve fetch [dep=2, id=3]
> mount B
StrictMode> unmount B
StrictMode> mount B

With npm i https://pkg.pr.new/jotai@3125:


> ⏳ start fetch [dep=0, id=1]
> ✅ resolve fetch [dep=0, id=1]
> mount A
StrictMode> unmount A
StrictMode> mount A
> ⏳ start fetch [dep=1, id=2]
> unmount A
> ✅ resolve fetch [dep=1, id=2]
> ⏳ start fetch [dep=2, id=3]
> ✅ resolve fetch [dep=2, id=3]
> mount B
StrictMode> unmount B
StrictMode> mount B

Copy link
Contributor

@a10nik a10nik Aug 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, I have a feeling now that even the first case I've mentioned doesn't really work in some cases. Working on a reproduction link now to prove the point

It all feels like just another implication of performing side effects in render: the code becomes dependent on the fact if render is called at a specific time or not. IMO it feels like a react-specific implementation detail. I know the talk that getters are supposed to be pure, though the fact that jotai supports async atoms and treats promises differently contradicts it in a sence, since starting any async operation, memoized or not, is strictly speaking a side-effect. Thus I take it as a jotai's fundamental design choice

Copy link
Contributor

@a10nik a10nik Aug 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Here's the link: https://stackblitz.com/edit/vitejs-vite-2rssqjqx?file=src%2FApp.tsx

What I'm trying to prove there is that it's generally impossible to optimally refresh a pending atom when its component is suspended. When the dep changes there is no way to tell:

  • if a component is suspended on some other longer promise => We would've wanted to recalculate its promise, however we can only judge about its suspension based on our own atom's promise state
  • if a component is still interested it the result of the promise => The component's render could be cancelled mid-suspense if some condition was switched in a parent render. My example doesn't demonstrate this effect, though I can do it if need be

Now it feels that optimizing for such cases isn't really a good idea and instead it would suffice to recommend subscribing to the atoms explicity outside of Suspense boundary if they really need that Suspense-time eagerness.

Copy link
Collaborator Author

@dmaskasky dmaskasky Aug 19, 2025

Choose a reason for hiding this comment

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

Jotai internals does not resolve promises, that is done in useAtomValue. In the future, I think it makes sense to stop unwrapping promises by default and to return the original promise from useAtomValue instead. Then, it would be up to the implementer to wrap with use.

  • Do not use use in useAtomValue (migration path: add { use: true } option which is true by default in v2, and warns it)

This is one of the ideas for v3 described in #2889

Copy link
Contributor

@a10nik a10nik Aug 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, thats true. I wasn't stating anything about jotai resolving promises internally, though. What I meant to say:

  • If we start async (therefore impure) operations in render, we make ourselves dependent on the render call times which react tries to keep a semi-private implementation detail.
  • Jotai now has pending promise checks when calculating dependencies which seems to be some optimization of the 2 cases I've mentioned, around suspending on an unmounted atom's promise -> dep change -> mount. (Actually, that's just my guess about the reasons, the actual reasoning behind the code might've been different, would love to know more)
  • Still, if we tried to optimize the 2 cases, our efforts are bound to fail in some cases or get outdated after some random react changes, as long as render stays impure. And it is generally impure now, when an atom is async, no matter if we await/use(promise) it or not. So even after { use: false } the renders will stay impure

I know and appreciate the idea about { use: false }. It will help with this situation to some extent, allowing us to additionally useAtomValue({ use: false }) outside of suspense boudary, where render-mount-unmount order is a bit more straightforward. It still would be prone to some corner cases unless we only do side-effects in useEffects as react wants us to.

for (const a of atomState.d.keys()) {
const aState = ensureAtomState(a)
aState.p.delete(atom)
}
// unmount dependencies
for (const a of atomState.d.keys()) {
const aMounted = unmountAtom(a)
Expand Down
49 changes: 49 additions & 0 deletions tests/vanilla/getMountedOrPendingDependents.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { describe, expect, it, vi } from 'vitest'
import { atom, createStore } from 'jotai/vanilla'

describe('getMountedOrPendingDependents consistent behavior', () => {
it('sub to asyncAtom -> syncAtom++', async () => {
const store = createStore()
const callback = vi.fn()
const syncAtom = atom(0)
const asyncAtom = atom((get) => {
get(syncAtom)
callback()
return new Promise((res) => setTimeout(res))
})

const unsub = store.sub(asyncAtom, () => {})
expect(callback).toHaveBeenCalledTimes(1)
callback.mockClear()
store.set(syncAtom, (v) => v + 1)
expect(callback).toHaveBeenCalledTimes(1)
callback.mockClear()
unsub()
expect(callback).toHaveBeenCalledTimes(0)
store.set(syncAtom, (v) => v + 1)
expect(callback).toHaveBeenCalledTimes(0)
})

it('sub to asyncAtom -> syncAtomWrapper -> syncAtom++', async () => {
const store = createStore()
const callback = vi.fn()
const syncAtom = atom(0)
const syncAtomWrapper = atom((get) => get(syncAtom))
const asyncAtom = atom((get) => {
callback()
get(syncAtomWrapper)
return new Promise((res) => setTimeout(res))
})

const unsub = store.sub(asyncAtom, () => {})
expect(callback).toHaveBeenCalledTimes(1)
callback.mockClear()
store.set(syncAtom, (v) => v + 1)
expect(callback).toHaveBeenCalledTimes(1)
callback.mockClear()
unsub()
expect(callback).toHaveBeenCalledTimes(0)
store.set(syncAtom, (v) => v + 1)
expect(callback).toHaveBeenCalledTimes(0)
})
})