Skip to content

Commit

Permalink
memory: fix 2 memory leaks in next-dev (#43859)
Browse files Browse the repository at this point in the history
This PR fixes two memory leaks I found debugging with @sokra.

## 1) Leak in `next-server.ts`

The first leak was caused by the fact that the `require.cache` associated to the `next-server` module was not being cleared up properly, so we leaked context from modules required in that page, like API routes.

## 2)  Leak with React Fetch

When evaluating a route, we also evaluated the `react.shared-subset.development.js` module where React patches the `fetch` function. The problem is that when re-evaluating a route as part of hot reloading, we were patching over the previously patched `fetch` function. 
The result of this operation meant that we were keeping a reference to the context of the previous `fetch` and thus to the previous route context, thus creating a memory leak, since we only needed the new context.

## Test plan

Checked manually the heap snapshots of a test app.

## Bug

- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Errors have a helpful link attached, see [`contributing.md`](https://github.com/vercel/next.js/blob/canary/contributing.md)

## Feature

- [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
- [ ] Related issues linked using `fixes #number`
- [ ] [e2e](https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs) tests added
- [ ] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have a helpful link attached, see [`contributing.md`](https://github.com/vercel/next.js/blob/canary/contributing.md)

## Documentation / Examples

- [ ] Make sure the linting passes by running `pnpm build && pnpm lint`
- [ ] The "examples guidelines" are followed from [our contributing doc](https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md)
  • Loading branch information
feedthejim committed Dec 8, 2022
1 parent 8a17198 commit 589f090
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 0 deletions.
Expand Up @@ -10,6 +10,8 @@ type WebpackPluginInstance = webpack.WebpackPluginInstance
const originModules = [
require.resolve('../../../server/require'),
require.resolve('../../../server/load-components'),
require.resolve('../../../server/next-server'),
require.resolve('../../../compiled/react-server-dom-webpack/client'),
]

const RUNTIME_NAMES = ['webpack-runtime', 'webpack-api-runtime']
Expand Down
15 changes: 15 additions & 0 deletions packages/next/server/dev/next-dev-server.ts
Expand Up @@ -109,6 +109,7 @@ export default class DevServer extends Server {
private edgeFunctions?: RoutingItem[]
private verifyingTypeScript?: boolean
private usingTypeScript?: boolean
private originalFetch?: typeof fetch

protected staticPathsWorker?: { [key: string]: any } & {
loadStaticPaths: typeof import('./static-paths-worker').loadStaticPaths
Expand Down Expand Up @@ -149,6 +150,7 @@ export default class DevServer extends Server {

constructor(options: Options) {
super({ ...options, dev: true })
this.persistPatchedGlobals()

This comment has been minimized.

Copy link
@edvinerikson

edvinerikson Mar 17, 2023

I think this runs too early. I am patching globalThis.fetch with a custom cache solution and nextjs removes it randomly. Same for msw interceptor.

My hacky solution:

const f = fetch;
  Object.defineProperty(globalThis, 'fetch', {
    get() {
      return f;
    },
    set() {
      // next dev server overrides msw and our cache patch
      // https://github.com/vercel/next.js/commit/589f090d0bdd6c6b0c1e1bcd8de7507037d4322e
    },
  });
this.renderOpts.dev = true
;(this.renderOpts as any).ErrorDebug = ReactDevOverlay
this.devReady = new Promise((resolve) => {
Expand Down Expand Up @@ -1385,6 +1387,14 @@ export default class DevServer extends Server {
return this.hotReloader!.ensurePage({ page: pathname, clientOnly: false })
}

private persistPatchedGlobals(): void {
this.originalFetch = global.fetch
}

private restorePatchedGlobals(): void {
global.fetch = this.originalFetch!
}

protected async findPageComponents({
pathname,
query,
Expand Down Expand Up @@ -1418,6 +1428,11 @@ export default class DevServer extends Server {
this.serverCSSManifest = super.getServerCSSManifest()
}
this.fontLoaderManifest = super.getFontLoaderManifest()
// before we re-evaluate a route module, we want to restore globals that might
// have been patched previously to their original state so that we don't
// patch on top of the previous patch, which would keep the context of the previous
// patched global in memory, creating a memory leak.
this.restorePatchedGlobals()

return super.findPageComponents({ pathname, query, params, isAppPath })
} catch (err) {
Expand Down

0 comments on commit 589f090

Please sign in to comment.