atomWithObservable unmounts unexepectedly

[dakom]

Summary

atomWithObservable unmounts unexepectedly when there is some async dependency. This is particularly bad when the observable is set via some loop, and this bug therefore also affects jotai-tanstack-query when fetchInterval is used.

It does not happen if there is no async dependency (See the "FIXME" note in the reproduction on line 18)

Link to reproduction

https://codesandbox.io/s/angry-bassi-r4326h?file=/src/App.tsx

See the console log, it gets spammed with mount/unmount/mount every tick

Additional notes

1. @dai-shi pointed out that the timeMount.onMount callback on line 27 never gets called
2. this does not occur in a simple reproduction with regular atoms (https://codesandbox.io/s/bitter-bird-yh9dm5?file=/src/App.tsx), so it may be a bug in just atomWithObservable

Check List

Please do not ask questions in issues.

• I've already opened a discussion before opening this issue, or already discussed in other media.

Please include a minimal reproduction.

• I've added a link to a typescript playground or codesandbox with a minimal reproduction.

[dai-shi]

This doesn't cause the behavior either:

  const [_, time] = await Promise.all([get(timeDepAtom), get(timeTriggerAtom)]);

---

I would not consider this a high priority, but am very curious how this happens. If anyone is interested, please feel free to dig into it.

[dakom]

yeah, interesting...

just to clarify - is this also not a high priority in usecases which depend on frequent ticks? i.e. is mount/unmount/mount extremely cheap and almost like a noop?

[dai-shi]

1. @dai-shi pointed out that the timeMount.onMount callback on line 27 never gets called

Never mind. I just forgot that onMount is only available with writable atoms. (TS should complain.)

[dakom]

hmmm, using Subject for the observable instead of BehaviorSubject also works: https://codesandbox.io/s/goofy-roman-x9476g?file=/src/App.tsx

[dai-shi]

just to clarify - is this also not a high priority in usecases which depend on frequent ticks? i.e. is mount/unmount/mount extremely cheap and almost like a noop?

In my mental model, mount/unmount isn't very cheap compared to read/write, so I'm not happy with it. In practice, it's probably cheap enough.

[dai-shi]

hmmm, using Subject for the observable instead of BehaviorSubject also works: https://codesandbox.io/s/goofy-roman-x9476g?file=/src/App.tsx

🤯

[dakom]

okay, I've narrowed it down a bit more, it's whenever there's an initial value set. In BehaviorSubject it's nuanced because it comes through the promise firing right away (and will actually fire twice), but this can also happen if you use a regular Subject and just set an initial value: https://codesandbox.io/s/frosty-shannon-2t9677?file=/src/App.tsx

[dakom]

edit: "recursion" -> "atoms driving themselves" .. in this case it's not exactly recursion, but rather a subscription mechanism that tries to manage itself via onMount callbacks

I tried attacking this from various angles... I was able to get this test to pass with a rewrite of atomWithObservable, but it broke everything else (see here if you're curious)

I tried a couple other techniques too. Long story short, I'm currently thinking that at least one of these is true:

1. atoms should not drive themselves, full stop. If any sort of looping is needed, it must be driven from the outside (e.g. via a store subscription).

2. there is something broken in the mount/unmounting logic somewhere and it's causing atoms to unmount too early

I'm curious to hear your thoughts, but fwiw without digging into the details of how the store/dependencies/mounting works - after playing with this code some more, I think it fundamentally feels wrong to have an atom drive itself and so I'm leaning towards 1 being the culprit. Although atomWithObservable is super nice, if it's fundamentally unsound (big if!), useObservableAsAtom() or something that registers the observable subscription in a hook is not that bad.

as for jotai-tanstack-query (and any other libs that depend on this, if any), yeah, this is potentially bad news... but there's a bit of funkiness with that approach as it is (i.e. the whole mechanism of caching between components isn't beneficial at all, jotai already solves that by sharing state between atoms). I think something like jotai-axios-query which does a single fetch (from the atom's point of view, of course it can retryOnFail until it resolves the promise), combined with a useRefetchInterval() effect to drive it is more sound.

Wondering what you think! I may be completely off here...

[dmaskasky]

I might be late to the party, but jotai-effect atomEffect supports recursion through set.recurse.

import { atomEffect } from 'jotai-effect'

const countAtom = atom(0)

atomEffect((get, set) => {
  // increments count once per second
  const count = get(countAtom)
  const timeoutId = setTimeout(() => {
    set.recurse(countAtom, increment)
  }, 1000)
  return () => clearTimeout(timeoutId)
})

[dai-shi]

There's still something I'm not 100% following, but here's some thoughts.

Basically, atomWithObservable is a hack. More precisely, it doesn't follow the Jotai principle, which is read function must be "pure" (= everything must be garbage collectable).
atomWithObservable violates that principle, because we can't peek the next value (it requires subscription). That said, it's fairly stable for typical use cases as we have been improving the implementation.

atoms should not drive themselves, full stop. If any sort of looping is needed, it must be driven from the outside (e.g. via a store subscription).

Can you clarify what's atoms deriving themselves?

there is something broken in the mount/unmounting logic somewhere and it's causing atoms to unmount too early

There's such a chance that mount/unmounting logic has bugs. It's pretty tricky especially with dependency atoms, and I'm not 100% confident with current code.

[dakom]

Thank you for sharing your insight, it's very helpful!

Can you clarify what's atoms deriving themselves?

Pretty much exactly what you said in other words :) But, I'd say that even this example is problematic:

const actionAtom = atom(null, (get, set, arg: string) => {
  console.log(`doing something with ${arg}`);
});
const timerAtom = atom(null, (get, set) => {
  setInterval(
    () => set(actionAtom, "next arg"),
    500
  );
});

I don't mean to take that comment out of the context of that discussion where it was only meant to illustrate the point of separating atoms, I just mean for real-world use where you want to clean up the timer, you only want one instance of it running at a time, etc., I think it's a footgun (if you disagree let me know please!)

I think generally the right way to use atoms as that the write semantics should always originate from the outside world into jotai atoms. Once you're in the world of atoms, it's fine to use write from atom to atom as a compositional tool, but most of the time you should really be read and just deriving one piece of data from another

I don't know the deep details, but I do think these issues are highly related to the difficulty of writing FRP libraries. I know it was dealt with extensively in the design of Rust's futures-signals with the separation of Mutable (which is like a write atom) and Signal (which is like a read atom). Different library and different API (i.e. I sortof lied, you can read from a Mutable, but, in the context of our discussion here it's more write-only) - but I think the idea of "mutating" vs. "mapping" being totally separate needs is relevant and very very hard to get 100% ergonomic and correct

I'm loving jotai btw, very happy to settle into a pattern of best practices and avoid the usage patterns that are less recommended, helps a lot to have clarity on what they are :)

[dai-shi]

Yeah, I knew that setInterval is problematic. It can return a clean up function and delegate the responsibility to the caller.

In general, you can put impure code in write such as mutating variables outside Jotai and communicating with servers, under your responsibility.

[dai-shi]

@tmkx Would you be interested in looking into this and/or #2007 (and #2008) ?

[tmkx]

const timeTriggerObservable = new BehaviorSubject(0);
const timeTriggerAtom = atomWithObservable(() => timeTriggerObservable);

timeTriggerAtom.onMount = () => {
  console.log("mount", performance.now());
  return () => console.log("unmount", performance.now());
};

export const timeDepAtom = atom(async () => "timeDep");

const timeAtom = atom(async (get) => {
  //FIXME: not awaiting this fixes the issue
  //i.e. this works (or just not using at all)
  //const _unusedDep = get(timeDepAtom);
  const _unusedDep = await get(timeDepAtom); // 😈 oops
  const time = await get(timeTriggerAtom);
  return time;
});

The timeTriggerAtom is stuck in a mount/unmount loop because when we await get(timeDepAtom), the timeAtom does not depend on timeTriggerAtom. As a result, timeTriggerAtom keeps getting unmounted.

So, should we unmount dependencies after the promise is fulfilled? (currently it's working in a sync manner, so it will clean dependencies when it encounters with the first await statement)

[dai-shi]

Thanks for the investigation!

should we unmount dependencies after the promise is fulfilled?

Oh, that sounds too tough. A promise may never fulfill.
Do you think we can fix it with atomWithObservable impl?
Or, is this easily reproducible with bare async atoms?

Using Promise.all can be a workaround, right? If we can't solve it, we could document the workaround.

[tmkx]

Using Promise.all can be a workaround, right?

yes!

[dai-shi]

ref: https://fosstodon.org/@daishi/111020604426323401

Okay, I think it's a doc issue then. Would you or anyone like to work on it?

[tien]

Think I was able to fix this issue here: #2631

Tested the changes against the codesandbox reproduction, no more infinite unmount/remount 👌

[tien]

Codesandbox link showcasing the fix: https://codesandbox.io/p/sandbox/still-shape-f9tg7n

[dmaskasky]

@tien Would you be able to look into this issue as well? Hopefully, this refactor could address this too.
jotaijs/jotai-scope#36

[tien]

I'll try to take a look, but I believe this is only fixable with changes to jotai-scope though.

[dmaskasky]

I'm happy to make changes to the library, but I am having trouble understanding what is going on. To me it seems, even if jotai-scope makes a copy of atomWithObservable, it should still be in sync with the observable.

[dmaskasky]

Happy to move this discussion out of this PR. Feel free to dm me on discord (dmaskasky#3642) or join the conversation in jotaijs/jotai-scope#36. Your help is much appreciated 🙏

[iwoplaza]

I recently had an idea for a jotai utility that would encourage defining dependencies in sync, even for async atoms. Here is the discussion thread. Maybe this would decrease the likelihood of encountering this edge-case in the first place.