vercel / next.js

The React Framework
https://nextjs.org
MIT License
125.49k stars 26.8k forks source link

`AsyncLocalStorage` and `res.locals` unexpected behavior starting in `13.4.3-canary.3` #53466

Open helloitsjoe opened 1 year ago

helloitsjoe commented 1 year ago

Verify canary release

Provide environment information

Operating System:
      Platform: darwin
      Arch: x64
      Version: Darwin Kernel Version 20.6.0: Thu Mar  9 20:39:26 PST 2023; root:xnu-7195.141.49.700.6~1/RELEASE_X86_64
    Binaries:
      Node: 20.2.0
      npm: 9.6.6
      Yarn: 3.6.1
      pnpm: N/A
    Relevant Packages:
      next: 13.4.12
      eslint-config-next: 13.4.12
      react: 18.2.0
      react-dom: 18.2.0
      typescript: N/A
    Next.js Config:
      output: N/A

NOTE: 13.4.12 is the most recent version I could run, all 13.4.13 canaries are crashing with the following error:

Failed to proxy http://127.0.0.1:0/about Error: connect EADDRNOTAVAIL 127.0.0.1 - Local (0.0.0.0:57514)

Which area(s) of Next.js are affected? (leave empty if unsure)

App Router

Link to the code that reproduces this issue or a replay of the bug

https://github.com/helloitsjoe/next-custom-server-repro

To Reproduce

Please see the reproduction repo, it has a full example of the setup described here and a more detailed description in the README:

  1. Create a custom server
  2. Create an instance of AsyncLocalStorage, and call .run() in custom server middleware
  3. Attach data to res.locals in custom server middleware
  4. From a file in the app directory, try to access the store instance with .getStore(), see that it's undefined
  5. From getServerSideProps in a file in the pages directory, try to access the store instance with .getStore(), see that it's undefined
  6. From getServerSideProps in a file in the pages directory, try to access res.locals, see that it's also undefined

Describe the Bug

The AsyncLocalStorage store is undefined in application code in the app directory, and res.locals is undefined in the pages directory (in getServerSideProps), even though they are available in the custom server at request time. This behavior started in next@13.4.3-canary.3.

Expected Behavior

I expect to have access to a globally defined AsyncLocalStorage store server-side in components in the app directory, and access to both the store and res.locals in getServerSideProps in the pages directory, without setting customServer: false. This is the behavior in next@13.4.2.

Which browser are you using? (if relevant)

115.0.5790.114 (Official Build) (x86_64)

How are you deploying your application? (if relevant)

No response

NEXT-1489

shaunwarman commented 1 year ago

@helloitsjoe - is this a safe option to cross boundaries of custom server and provide that context to server component pre-render? Any worries of interleaving requests and mixing up user data here? cc @coltonehrman

This reminds me of the issues that came with https://github.com/othiym23/node-continuation-local-storage and not explicitly passing context object through middleware.

dhalbrook commented 1 year ago

Still seeing this on 13.4.18

tatethurston commented 1 year ago

I ran into this trying to use AsyncLocalStorage. The instance of AsyncLocalStorage needs to be shared across Next runtime contexts using globalThis (seen in the linked repro case above).

Did something change with the handling of globalThis across next contexts in 13.4.2?

stychu commented 11 months ago

@tatethurston Did you find it working? I face the same issue with custom express server that the asynclocalstorage is not visible on the NextJs runtime context but works no problem with Express server context. I wanted to setup this for request tracing in logs but logs that I put in nextjs API or RSC don't share the asynclocalstorage that is initialized from custom express server ;(

stychu commented 11 months ago

This works for me with latest Nextjs 13/14

/* eslint-disable no-var */

// TODO Look into AsyncContext instead AsyncLocalStorage
//  for performance improvements once its implemented in javascript runtime
//  https://github.com/tc39/proposal-async-context
/*
 * Set tracing store on globalThis so asyncLocalStorage
 * can work in both NextJs and Express runtime contexts
 */
import { AsyncLocalStorage } from 'node:async_hooks';

import type { NextFunction } from 'express';

declare global {
  var tracingRequestGlobalStore: AsyncLocalStorage<Tracing>;
}

type Tracing = {
  requestTraceId: string;
  persistentTraceId: string;
};

if (!globalThis.tracingRequestGlobalStore) {
  globalThis.tracingRequestGlobalStore = new AsyncLocalStorage<Tracing>();
}

export const getTracingRequestStore = (): Tracing | undefined =>
  globalThis.tracingRequestGlobalStore.getStore();

export const setTracingRequestStore = (
  tracing: Tracing,
  next: NextFunction,
) => {
  globalThis.tracingRequestGlobalStore.run(tracing, next);
};
tatethurston commented 11 months ago

@stychu yes in next versions prior to 13.4.2

stychu commented 11 months ago

@tatethurston Im using "next": "14.0.2-canary.10", and its working also on 13.5.6 it worked for me too

shaunwarman commented 11 months ago

@coltonehrman

coltonehrman commented 11 months ago

@tatethurston Im using "next": "14.0.2-canary.10", and its working also on 13.5.6 it worked for me too

Do you have a repo to reproduce a working example? I can't seem to get my setup working on either of these versions

stychu commented 4 months ago

@helloitsjoe @coltonehrman @tatethurston I completely forgot about replying to the issue. I apologise guys. Dunno if you found the solution or ditched this completely but your issue is within the start-server.js script.

If you upgrade to the latest nextjs version 14.2.3 and change the server script to this it should work.

const { createServer } = require("http");
const next = require("next");
const express = require("express");
const { getStore, run } = require("./async-store");

console.log("start-server pid", process.pid);

const app = express();

const dev = process.env.NODE_ENV !== "production";
const setCustomServerFalse = process.env.SET_CUSTOM_SERVER_FALSE === "true";

console.log("setCustomServerFalse", setCustomServerFalse);

const nextApp = next({
  dev,
  // If customServer is not explicitly set to false, don't include it at all
  ...(setCustomServerFalse ? { customServer: false } : null),
});
const handle = nextApp.getRequestHandler();

nextApp.prepare().then(() => {
  console.log("NextApp is ready");

// Middleware to set up AsyncLocalStorage for passing context around
  app.use((req, res, next) => {
    const store = getStore();
    // Create a context for the request.
    // `run()` sets the AsyncLocalStorage instance's `enabled` to true
    run(new Map(), next);
  });

// Middleware to set up res.locals for passing context around
  app.use((req, res, next) => {
    res.locals.foo = "bar";
    next();
  });

  app.get("*", (req,res,next) => {
    handle(req,res).catch(next)
  });

  createServer(app).listen(3000, () => {
    console.log("listening on http://localhost:3000");
  });
});
Screenshot 2024-05-09 at 12 25 22 Screenshot 2024-05-09 at 12 25 28

The custom server setup need to be done within the nextapp prepare handler plus getRequestHandler should be called explicitly with req,res instead of passing it as a function to the express handler.

acrognale commented 4 months ago

The globalThis method as mentioned by @stychu works for me with Next 14.2.3

joaquinbentancor commented 3 months ago

I'm trying to wrap the middleware with AsyncLocalStorage to have a trace id in every request, this works fine at middleware level but when I try to retrieve the context info in a Route Handler, the context is undefined. Could be a bind problem? How can I tackle the problem?