Skip to content
This repository has been archived by the owner on Jan 1, 2025. It is now read-only.

Atoms with a selector default cause memory leak #1840

Closed
noritheduck opened this issue Jun 6, 2022 Discussed in #1821 · 0 comments
Closed

Atoms with a selector default cause memory leak #1840

noritheduck opened this issue Jun 6, 2022 Discussed in #1821 · 0 comments
Assignees

Comments

@noritheduck
Copy link

Discussed in #1821

Originally posted by janicduplessis May 24, 2022

Issue

Creating an atom with a selector to lazily initialize the default value like mentioned in the documentation here will cause all values set to it to leak.

This will leak:

atom({
  key: "transactions",
  default: selector({
    key: "transactions/default",
    get: retrieveTransactions
  })
});

This is fine:

atom({
  key: "transactions",
  default: retrieveTransactions()
});

The issue is caused by the selector created by atomWithFallback (https://github.com/facebookexperimental/Recoil/blob/main/packages/recoil/recoil_values/Recoil_atom.js#L661) which will cause all set on the atom to be cached.

Suggestion

Disabling cache for the selector created by atomWithFallback fixes the problem and should not cause any issue as I don't see why we would want to keep old values for this selector. It is also very cheap to execute.

   const sel = Recoil_selector({
     key: `${options.key}__withFallback`,
+    cachePolicy_UNSTABLE: { eviction: 'most-recent' },
     get: ({
       get
     }) => {

If this makes sense I can open a PR to make this change.

Repro

https://codesandbox.io/s/clever-wilbur-8cv6qr?file=/src/App.js

Open chrome devtools and go to the Memory tab. Notice ever increasing memory usage for the .csb.app js context.

Screen Shot 2022-05-24 at 8 28 36 PM

By taking a snapshot we can clearly see the leaked Transaction objects.

Screen Shot 2022-05-24 at 8 30 10 PM

Set LEAK to false and refresh

Observe no more memory leak

@noritheduck noritheduck self-assigned this Jun 6, 2022
noritheduck pushed a commit to noritheduck/Recoil that referenced this issue Jun 7, 2022
Summary:
- Wrapping selector does not need to cache because the selector being wrapped has caching options
- Related to facebookexperimental#1840

Reviewed By: drarmstr

Differential Revision: D36956571

fbshipit-source-id: 2709b8603f94158f6a7843e743d1311ff569685d
facebook-github-bot pushed a commit that referenced this issue Jun 7, 2022
Summary:
Pull Request resolved: #1844

- Wrapping selector does not need to cache because the selector being wrapped has caching options
- Related to #1840

Reviewed By: drarmstr

Differential Revision: D36956571

fbshipit-source-id: 1c5729e96ffdbcf94c3afdd8ce42648d4148e1b6
@drarmstr drarmstr closed this as completed Jun 7, 2022
snipershooter0701 pushed a commit to snipershooter0701/Recoil that referenced this issue Mar 5, 2023
Summary:
Pull Request resolved: facebookexperimental/Recoil#1844

- Wrapping selector does not need to cache because the selector being wrapped has caching options
- Related to facebookexperimental/Recoil#1840

Reviewed By: drarmstr

Differential Revision: D36956571

fbshipit-source-id: 1c5729e96ffdbcf94c3afdd8ce42648d4148e1b6
eagle2722 added a commit to eagle2722/Recoil that referenced this issue Sep 21, 2024
Summary:
Pull Request resolved: facebookexperimental/Recoil#1844

- Wrapping selector does not need to cache because the selector being wrapped has caching options
- Related to facebookexperimental/Recoil#1840

Reviewed By: drarmstr

Differential Revision: D36956571

fbshipit-source-id: 1c5729e96ffdbcf94c3afdd8ce42648d4148e1b6
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants