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.

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

[NextJS] Latest browser updates appear to have changed behavior of snapshots #1936

Closed
pcreehan opened this issue Aug 5, 2022 · 10 comments
Closed

Comments

@pcreehan
Copy link

pcreehan commented Aug 5, 2022

Starting last night (8/4/2022), Our DebugObserver component started throwing on useRecoilSnapshot with the error that the snapshot has already been released.
image
This reproduces on new versions of browsers only...installing older versions (and testing before they auto-update), we do not get this exception with the exact same code. This is the only place in our code we're using snapshots directly.

const DebugObserver = () => {
  const snapshot = useRecoilSnapshot();
  useEffect(() => {
    console.info('The following atoms were modified:');
    for (const node of snapshot.getNodes_UNSTABLE({ isModified: true })) {
      console.info(node.key, snapshot.getLoadable(node));
    }
  }, [snapshot]);

  return null;
};

I tried reverting to older builds of recoil, running the code on other machines, different browsers, and my coworkers see the same behavior this morning.
Our application is build with NextJs...here are our dependencies:

 "dependencies": {
    "@date-io/date-fns": "^2.14.0",
    "@emotion/css": "^11.9.0",
    "@emotion/react": "^11.9.0",
    "@emotion/styled": "^11.8.1",
    "@fontsource/roboto": "^4.5.7",
    "@mui/icons-material": "^5.8.3",
    "@mui/lab": "^5.0.0-alpha.76",
    "@mui/material": "^5.8.3",
    "@mui/x-data-grid": "^5.8.0",
    "@mui/x-data-grid-generator": "^5.12.0",
    "@mui/x-date-pickers": "^5.0.0-alpha.6",
    "@newrelic/next": "^0.2.0",
    "axios": "^0.27.2",
    "color-parse": "^1.4.2",
    "cookies": "^0.8.0",
    "cron-parser": "^4.4.0",
    "cronstrue": "^2.9.0",
    "d3": "^7.4.4",
    "date-fns": "^2.28.0",
    "intercept-stdout": "^0.1.2",
    "lodash": "^4.17.21",
    "newrelic": "^8.15.0",
    "next": "^12.2.2",
    "qs": "^6.10.5",
    "react": "^18.2.0",
    "react-dom": "^18.2.0",
    "react-error-boundary": "^3.1.4",
    "react-number-format": "^4.9.3",
    "recoil": "^0.7.4",
    "rrule": "^2.7.0",
    "uuid": "^8.3.2",
    "validator": "^13.7.0",
    "victory": "^36.5.3"
  },
  "devDependencies": {
    "@babel/plugin-transform-modules-commonjs": "^7.18.2",
    "@netlify/plugin-nextjs": "^4.12.2",
    "@testing-library/jest-dom": "^5.16.4",
    "@testing-library/react": "^13.3.0",
    "@testing-library/user-event": "^14.2.0",
    "@types/cookies": "^0.7.7",
    "@types/d3": "^7.4.0",
    "@types/jest": "^28.1.1",
    "@types/lodash": "^4.14.182",
    "@types/node": "^17.0.41",
    "@types/react": "^18.0.12",
    "@types/react-dom": "^18.0.5",
    "@types/uuid": "^8.3.4",
    "@types/validator": "^13.7.4",
    "@typescript-eslint/eslint-plugin": "^5.27.1",
    "@typescript-eslint/parser": "^5.27.1",
    "babel-jest": "^28.1.1",
    "concurrently": "^7.2.1",
    "cross-env": "^7.0.3",
    "eslint": "^8.21.0",
    "eslint-config-next": "^12.2.4",
    "eslint-config-prettier": "^8.5.0",
    "eslint-plugin-jest-dom": "^4.0.2",
    "eslint-plugin-prettier": "^4.2.1",
    "eslint-plugin-react-hooks": "^4.6.0",
    "eslint-plugin-testing-library": "^5.6.0",
    "jest": "^28.1.1",
    "jest-environment-jsdom": "^28.1.1",
    "jsdom": "^19.0.0",
    "next-router-mock": "^0.6.10",
    "prettier": "^2.7.1",
    "replace-in-file": "^6.3.5",
    "sass": "^1.52.3",
    "swagger-typescript-api": "^9.3.1",
    "ts-jest": "^28.0.4",
    "typescript": "^4.7.3"
  }

I'd be surprised if we're the only ones experiencing odd new behavior today.

EDIT: I should add that this only repros when running next dev but works when running next start. I'm not sure if this implies a problem with nextjs with the new browser update or a problem with Recoil, or something that surfaces only when using Recoil in NextJs.

@drarmstr drarmstr changed the title Latest browser updates appear to have changed behavior of snapshots [NextJS] Latest browser updates appear to have changed behavior of snapshots Aug 9, 2022
@drarmstr
Copy link
Contributor

drarmstr commented Aug 9, 2022

I'm not familiar with the NextJS internals but it may be using React's Fast Refresh. Does #1891 resolve your issue? That isn't part of a public release yet but should be in the nightly branch.

@Vija02
Copy link

Vija02 commented Aug 9, 2022

Experiencing the same. The nightly branch doesn't solve the issue.
I can reproduce the bug on:

  • Firefox 103.0.1
  • Chrome 104.0.5112.79

It's not an issue on Chrome 103.0.5060.134-1.
Also using Next.JS which uses Fast Refresh + SSR hydration.

@drarmstr I tried to debug and compare with the working Chrome version and the difference seems to be in the autoRelease_INTERNAL function. The timing on when the release function is called seems to have changed. Seems like it's released prematurely resulting in the error.

@Vija02
Copy link

Vija02 commented Aug 9, 2022

Digging down the rabbit hole, I found the culprit for Chrome.
Turns out Chrome 104 made it so that useTimeout works with time of 0. Previously, it was always clamped at 1. Here's the change log. This is the commit hash: 330e7e8111a146e1e69ace46cf07e43943fc75d0

Setting the timeout to 1 in the autoRelease_INTERNAL function fixes this issue (and is equal to the old behavior).

However, the issue still persists in Firefox. I tested a few older version including v86 and the issue persists. Has this always been an issue? I'm running Firefox, Next.js 12.2.4, React 18.2.0 and nightly Recoil. Not sure if this happens on older React/Next.js versions since this is the first time I used RecoilSync (or useRecoilSnapshot).


I tried tracing _refCount but I'm not too sure how some parts work. Here's some findings:

  • The first part of the page load are consistent. The count is 2 due to:
    • useRecoilSnapshot first render
    • Creates a new snapshot with a count of 1 (not yet removed by autorelease) (0 -> 1)
    • The temporary retain (below useEffect) added, then remove a count after passing it
    • The retain that lives during the component live time (inside the useEffect in useRecoilSnapshot) (1 -> 2 -> 3 -> 2 -> 1)
  • Then the useRecoilSnapshot is re-rendered

This is where it starts to differ. Chrome:

  • useRecoilSnapshot is re-rendered again
  • Creates a new snapshot again (0 -> 1)
  • Auto release 1 count from the old snapshot (2 -> 1)
  • Temporary retain (below useEffect) (1 -> 2)
  • Auto release another count (2 -> 1)
  • Release in useEffect (1 -> 0)
  • Retain in useEffect (1 -> 2)
  • Release temporary (2 -> 1)

I'm at a lost at how the count works once the new snapshot become part of the equation. The number does add up if there's 2 different Snapshot instance.

In Firefox, continuing from the first common part:

  • Auto release a count (2 -> 1)
  • Creates a new snapshot (0 -> 1)
  • Auto release (1 -> 0)
  • useRecoilSnapshot is re-rendered (with 0 count)
  • During the temporary retain (below useEffect), it tries to retain and spit out the error since there's no more count.

I hope that somewhat helps. I'm not too sure how the Snapshot cloning works.

At the end of the day, setting the timeout to a higher number (eg: 10) seems to fix it for Firefox. So I'll be doing that as a fix for now since I've spent a day on this 😂

@drarmstr
Copy link
Contributor

Ah, thanks for diving into this! There are a few other places with setTimeout() such as in useRecoilSnapshot() directly. I should be able to update the timeouts to 10 to workaround the FireFox ordering issue..

drarmstr added a commit to drarmstr/Recoil that referenced this issue Aug 10, 2022
Summary:
Resolves facebookexperimental#1936

It appears that some browsers changed behavior for `setTimeout(..., 0);`.  The timeout previously had a minimum value of 1 but now has new behavior for 0.  This caused code ordering changes which broke snapshot auto release for some browsers, though likely only for dev mode when React Fast Refresh was being used (tested by Vija02 with Chrome 104.0.5112.79 and Firefox 103.0.1 using NextJS dev mode). Update the timeouts to workaround this issue.

Differential Revision: D38591482

fbshipit-source-id: 278f101b4734d039409abc963bc0d1df9277a748
@drarmstr
Copy link
Contributor

@Vija02 / @pcreehan - Added workaround in #1943 if you are able to test and confirm?

drarmstr added a commit to drarmstr/Recoil that referenced this issue Aug 10, 2022
…tal#1943)

Summary:
Pull Request resolved: facebookexperimental#1943

Resolves facebookexperimental#1936

It appears that some browsers changed behavior for `setTimeout(..., 0);`.  The timeout previously had a minimum value of 1 but now has new behavior for 0.  This caused code ordering changes which broke snapshot auto release for some browsers, though likely only for dev mode when React Fast Refresh was being used (tested by Vija02 with Chrome 104.0.5112.79 and Firefox 103.0.1 using NextJS dev mode). Update the timeouts to workaround this issue.

Differential Revision: https://www.internalfb.com/diff/D38591482?entry_point=27

fbshipit-source-id: 584741ec055bd9f1d34adf0fe721191d78e49d9b
@pcreehan
Copy link
Author

@Vija02 / @pcreehan - Added workaround in #1943 if you are able to test and confirm?

This seems to fix it from my limited testing. It wasn't hard to repro before and I don't see it occurring with your branch running.

@drarmstr
Copy link
Contributor

0.7.5 released

@madebyherzblut
Copy link

I can also confirm that 0.7.5 fixed the issue for me in Firefox. Thanks!

@PiR1
Copy link

PiR1 commented Sep 16, 2022

I still have this bug on Firefox (105.0b9).
If I set the timeout delay to 100ms it works but I think it will depends of the computer and app performances 😕

@chriszrc
Copy link

Yes, I had to pnpm patch autoRelease_INTERNAL() function as well with a 100ms timeout to get this working in firefox as well-

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants