Anchor the async local storage instances to global symbols - #97255
Draft
unstubbable wants to merge 1 commit into
Draft
Anchor the async local storage instances to global symbols#97255unstubbable wants to merge 1 commit into
unstubbable wants to merge 1 commit into
Conversation
The six async local storages must be singletons within a realm. A store entered through one reference has to be readable through every other reference, otherwise code running inside the scope sees no store at all. Until now that relied on module identity, which is a weaker guarantee than the requirement. A realm evaluates the same `next` file more than once if the package is reachable through more than one path, and each evaluation then created a storage of its own. The `.external.js` rewrite in `handle-externals.ts` pins which specifier every layer requires, but it cannot help when one specifier resolves to two filenames. That is what we hit intermittently in `next dev` with Cache Components. A bug in Node's `fs.realpathSync` can return a path with its symlinks unresolved, and the module loader keys the module cache on that path, so on a pnpm install `next/dist/...` is evaluated twice. A Route Handler calling `revalidatePath` then read a `workAsyncStorage` that nothing had ever entered and crashed, and `io()` read a `workUnitAsyncStorage` that was not the one `app-render` had entered, so sync IO went untracked. The Node fix is nodejs/node#65113, which is not in a release yet, and versions without it stay affected once it is. Each instance is now anchored to a global symbol, which holds the singleton for any number of copies. Worker threads and edge sandboxes still get their own storages, because each has its own `globalThis`. The key includes the Next.js version, so a realm that holds two different versions of Next.js keeps them apart rather than letting one version read a store that the other shaped, which we cannot assume is compatible. The helper itself is stateless and all state lives on `globalThis`, so `async-local-storage.ts` being duplicated along with everything else does not matter. `getOrCreateGlobalAsyncLocalStorage` also replaces the equivalent code in `request-insights-identity.ts`, which was already anchoring its storage this way. The tests that skipped this under Turbopack are enabled again, and they are what covers the fix. In `dev-warmup.util.ts` both `testInitialLoad` and `testNavigation` returned early under Turbopack, before the revalidation and every assertion after it, and the two `sync IO` tests were never registered at all, which takes that suite from 72 to 88 tests under Turbopack. `cache-components-tasks.test.ts` had the same two early returns. This does not make duplicate module instances go away, it only removes the consequence that is fatal. A realm that loads `next/dist` twice still pays for two module registries and twice the memory.
Contributor
Tests PassedCommit: 2801990 |
Contributor
Stats from current PR🔴 2 regressions
📊 All Metrics📖 Metrics GlossaryDev Server Metrics:
Build Metrics:
Change Thresholds:
⚡ Dev Server
📦 Dev Server (Webpack) (Legacy)📦 Dev Server (Webpack)
⚡ Production Builds
📦 Production Builds (Webpack) (Legacy)📦 Production Builds (Webpack)
📦 Bundle SizesBundle Sizes⚡ TurbopackClient Main Bundles
Server Middleware
Build DetailsBuild Manifests
Build Cache
📦 WebpackClient Main Bundles
Polyfills
Pages
Server Edge SSR
Middleware
Build DetailsBuild Manifests
Build Cache
🔄 Shared (bundler-independent)Runtimes
📝 Changed Files (21 files)Files with changes:
View diffsapp-page-exp..ntime.dev.jsfailed to diffapp-page-exp..time.prod.jsDiff too large to display app-page-tur..ntime.dev.jsfailed to diffapp-page-tur..time.prod.jsDiff too large to display app-page-tur..ntime.dev.jsfailed to diffapp-page-tur..time.prod.jsDiff too large to display app-page.runtime.dev.jsfailed to diffapp-page.runtime.prod.jsDiff too large to display app-route-ex..ntime.dev.jsDiff too large to display app-route-ex..time.prod.jsDiff too large to display app-route-tu..ntime.dev.jsDiff too large to display app-route-tu..time.prod.jsDiff too large to display app-route-tu..ntime.dev.jsDiff too large to display app-route-tu..time.prod.jsDiff too large to display app-route.runtime.dev.jsDiff too large to display app-route.ru..time.prod.jsDiff too large to display pages-api.runtime.dev.jsDiff too large to display pages.runtime.dev.jsDiff too large to display server.runtime.prod.jsDiff too large to display pages-api-tu..time.prod.jsDiff too large to display pages-turbo...time.prod.jsDiff too large to display 📎 Tarball URLCommit: 2801990 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The six async local storages must be singletons within a realm. A store entered through one reference has to be readable through every other reference, otherwise code running inside the scope sees no store at all. Until now that relied on module identity, which is a weaker guarantee than the requirement. A realm evaluates the same
nextfile more than once if the package is reachable through more than one path, and each evaluation then created a storage of its own. The.external.jsrewrite inhandle-externals.tspins which specifier every layer requires, but it cannot help when one specifier resolves to two filenames.That is what we hit intermittently in
next devwith Cache Components. A bug in Node'sfs.realpathSynccan return a path with its symlinks unresolved, and the module loader keys the module cache on that path, so on a pnpm installnext/dist/...is evaluated twice. A Route Handler callingrevalidatePaththen read aworkAsyncStoragethat nothing had ever entered and crashed, andio()read aworkUnitAsyncStoragethat was not the oneapp-renderhad entered, so sync IO went untracked. The Node fix is nodejs/node#65113, which is not in a release yet, and versions without it stay affected once it is.Each instance is now anchored to a global symbol, which holds the singleton for any number of copies. Worker threads and edge sandboxes still get their own storages, because each has its own
globalThis. The key includes the Next.js version, so a realm that holds two different versions of Next.js keeps them apart rather than letting one version read a store that the other shaped, which we cannot assume is compatible. The helper itself is stateless and all state lives onglobalThis, soasync-local-storage.tsbeing duplicated along with everything else does not matter.getOrCreateGlobalAsyncLocalStoragealso replaces the equivalent code inrequest-insights-identity.ts, which was already anchoring its storage this way.The tests that skipped this under Turbopack are enabled again, and they are what covers the fix. In
dev-warmup.util.tsbothtestInitialLoadandtestNavigationreturned early under Turbopack, before the revalidation and every assertion after it, and the twosync IOtests were never registered at all, which takes that suite from 72 to 88 tests under Turbopack.cache-components-tasks.test.tshad the same two early returns.This does not make duplicate module instances go away, it only removes the consequence that is fatal. A realm that loads
next/disttwice still pays for two module registries and twice the memory.