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

pcreehan commented 2 years ago

Starting last night (8/4/2022), Our DebugObserver component started throwing on useRecoilSnapshot with the error that the snapshot has already been released.

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

pcreehan commented 2 years ago

@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 commented 2 years ago

0.7.5 released

madebyherzblut commented 2 years ago

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

PiR1 commented 2 years ago

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 commented 3 months ago

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

facebookexperimental / Recoil

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