fix(utils): fix memory leaks and other issues in atomWithObservable

[tobias-walle]

@dai-shi As discussed this PR builds on #1157

• I added more tests for some of the edge cases
• I fixed some known and unknown issues that are covered by the new tests

This is the test result with the current implementation:

This is the test result new implementation:

Fixed issues:

• Emitted values between the initialValue and onMount are no longer ignored
• Falsy initial values are no longer ignored (0, false, '', etc.)
• If the observable is an BehaviourSubject or already has a value, Suspense is no longer required.
• If the observable never emits and no initial value is provided, the subscription is cancelled after 10 seconds. This value can be configured with the timeout option
  • I implemented this solution, based on your recommendation in fix(utils): Initial data for atomWithObservable  #1058 (comment). Unfortunately I also didn't find a better solution.
  • I choose a timeout of ten seconds as a default, instead of one second, because it is not uncommon for requests, to take more than one second.
  • We might want to log a warning during development if the timeout gets applied. This way the user knows that something went wrong and can fix the issue (What are your thoughts on that?)
  • This is actually a breaking change. Because it will break observables that take more than ten seconds to emit their first value without manually setting or disabling the timeout. If this is not okay for you, we can remove the default for now and add it separately in the next major release.

I'm looking forward to your thoughts and feedback!

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
jotai	✅ Ready (Inspect)	Visit Preview	May 23, 2022 at 4:12PM (UTC)

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ed10d98:

Sandbox	Source
React	Configuration
React Typescript	Configuration
React Browserify	Configuration
React Snowpack	Configuration
Next.js	Configuration
Next.js with custom Babel config	Configuration
React with custom Babel config	Configuration

[dai-shi]

Hi, thanks for working on it. I'll have a look later.
Meanwhile, I just merged #1157. So, you can update it here.

[tobias-walle]

Hi, thanks for working on it. I'll have a look later. Meanwhile, I just merged #1157. So, you can update it here.

I updated the MR.

To my earlier point:

We might want to log a warning during development if the timeout gets applied. This way the user knows that something went wrong and can fix the issue (What are your thoughts on that?)

I don't think that this would be a good idea anymore. I think in some scenarios the timeout just cannot be avoided and in these cases warning logs would be pretty annoying.

[tobias-walle]

I see that a test is failing. I will look into it later (probably tomorrow).

[dai-shi]

If the observable never emits and no initial value is provided, the subscription is cancelled after 10 seconds.

You seem to be misunderstanding something, at least from my understanding. I would expect to clear timeout on mount. So, 1 second is mostly enough (or we don't know for sure anyways). It's not about first emitting.

[tobias-walle]

@dai-shi My understanding was, if the first value doesn't emit the "loading" indicator from the suspense is always shown.

This way the onMount effect gets never called, as the component that uses useAtom is never rendered (It just throws the promise). If the component now gets removed before the first render/first emit, it will always leak the subscription, independent from the time that has passed.

This test should reproduce the behaviour:

jotai/tests/utils/atomWithObservable.test.tsx

Lines 246 to 277 in dfb6262

	it('cleanup subscriptions after timeout if initial value never emits', async () => {
	const subject = new Subject()
	let subscriptions = 0
	const observable = new Observable((subscriber) => {
	subscriptions++
	subject.subscribe(subscriber)
	return () => {
	subscriptions--
	}
	})
	const observableAtom = atomWithObservable(() => observable, { timeout: 500 })
	const Counter = () => {
	const [state] = useAtom(observableAtom)
	return <>count: {state}</>
	}
	const { findByText, rerender } = render(
	<Provider>
	<Suspense fallback="loading">
	<Counter />
	</Suspense>
	</Provider>
	)
	await findByText('loading')
	expect(subscriptions).toEqual(1)
	rerender(<div />)
	await waitFor(() => expect(subscriptions).toEqual(0))
	})

This is always throws:
https://github.com/pmndrs/jotai/blob/main/src/core/useAtomValue.ts#L52
https://github.com/pmndrs/jotai/blob/main/src/core/useAtomValue.ts#L32

So this never gets called (which calls onMount):
https://github.com/pmndrs/jotai/blob/main/src/core/useAtomValue.ts#L76

But I will test my assumptions again and create a code sandbox to reproduce the memory leak in a real app. Maybe this was a misunderstanding on my side.

[dai-shi]

if the first value doesn't emit the "loading" indicator from the suspense is always shown.

Oh, yeah. You are right. Hm, what was I confused with..
It's different from atomWithSubscription in jotai/urql.

Okay, in such a case, I think the default behavior is no timeout, pending infinitely.
timeout option is just optional (so that developers can avoid memory leaks if they want).

[tobias-walle]

if the first value doesn't emit the "loading" indicator from the suspense is always shown.

Oh, yeah. You are right. Hm, what was I confused with.. It's different from atomWithSubscription in jotai/urql.

Okay, in such a case, I think the default behavior is no timeout, pending infinitely. timeout option is just optional (so that developers can avoid memory leaks if they want).

Okay, I will update the default accordingly 👍

[tobias-walle]

@dai-shi I just figured out, that the subscription only get's leaked if the Provider is destroyed before the the first value is emitted.

As this is a pretty uncommon edge case and the timeout can introduce other bugs, I decided to just completely remove it. This also makes the code a little less complicated.

We could avoid the leak completely if the Provider would run a callback on all atoms, once it unmounts. But as this is a very uncommon issue, I wouldn't give this any priority at the moment.

I also fixed the tests. So this MR should be ready.

[dai-shi]

that the subscription only get's leaked if the Provider is destroyed before the the first value is emitted.

This is only true in React 17. React 18 can invoke render multiple time before comitting for various reasons. jotai atom's read function is same as the render function. So, technically it has to be "pure". https://gist.github.com/sebmarkbage/75f0838967cd003cd7f9ab938eb1958f#what-does-pure-mean-in-react

See more here: facebook/react#15317

I decided to just completely remove it.

That said, it's fine for now, but I'd like to leave some comments about the memory leak risk.

[dai-shi]

Some minor comments.

[tobias-walle]

That said, it's fine for now, but I'd like to leave some comments about the memory leak risk.

Should I add the comments in the code or in the documentation?

[dai-shi]

Should I add the comments in the code or in the documentation?

In the code, please.

[tobias-walle]

Should I add the comments in the code or in the documentation?

In the code, please.

I added the comment.

[dai-shi]

Looks good. Thanks for your contribution.
I'll merge it when we are ready to cut a new release.