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

CLS Context is lost after using multer middleware #814

Open
josencv opened this issue Dec 12, 2019 · 4 comments · May be fixed by #1124
Open

CLS Context is lost after using multer middleware #814

josencv opened this issue Dec 12, 2019 · 4 comments · May be fixed by #1124

Comments

@josencv
Copy link

josencv commented Dec 12, 2019

Hello! I'm using the cls-hooked library, which let's me use continuous local storage in my express app. After I use the multer.file middleware, my context is lost.

I patched the middleware using the solution proposed here and now it works fine. I'm letting you know just in case you want to fix it.

Cheers,

Additional info
Node version: 10.15.3
Express version: 4.16.3
Multer version: 1.4.2
CLS Hooked version: 4.2.2
OS: Ubuntu 18.04.3 LTS

@Tchoupinax
Copy link

Tchoupinax commented Nov 23, 2020

Hello,

I have the same issue with the object AsyncLocalStorage (from 'async_hooks'), the context is lost on the route which has the multer middleware.

Edit: https://medium.com/@theekshanawj/nodejs-using-multer-and-cls-hooked-together-a00decbebab6 fixes my problem ^^

@grzegorzjudas
Copy link

grzegorzjudas commented Dec 2, 2020

I struggled for a long time with the same issue, but I was able to go around it by passing all necessary middlewares with the following notation:

async function uploadMiddleware (req: Request, res: Response, next: NextFunction) {
    try {
        await new Promise<void>((resolve, reject) => {
            multer({
                // ...
            }).array('files')(req, res, (error: any) => {
                if (error) return reject(error);

                return resolve();
            });
        });

        next();
    } catch (error) {
        next(error);
    }
}

server.get('/endpoint', uploadMiddleware, controller);

So basically what I'm doing is, I'm running the multer middleware in-directly by wrapping it in another middleware that handles context correctly and then, at correct time, continuing to next middlewares/controller by calling next().

Edit: just noticed it's basically the same solution as in the aforementioned medium.com article. So... yay me :) having it directly here may help people stumbling here on this problem.

@ajhyndman
Copy link

I just thought I'd share a cheap user-space workaround that my team came up with, in case it's helpful for anyone else.

const { AsyncResource } = require('node:async_hooks')

/**
 * Ensure that an express middleware respects async local storage context.
 *
 * Accepts an express middleware and returns a copy of that middleware with the
 * next() call explicitly bound to the current AsyncResource context.
 *
 * When working with native promises or async/await syntax, Node can
 * automatically forward any existing async resource to nested async or promise
 * invocations. However, when working with nested callback-passing patterns
 * (such as occurs in an express middleware stack) Node cannot know whether the
 * function being passed around should be bound to the current resource.
 *
 * Express may update their plugin architecture to resolve this in a future
 * version. Middleware developers can also update their implementations to
 * ensure backward-compatible support for AsyncLocalStorage. This function
 * allows us to work around the issue for current versions of express and
 * plugins that have not yet been updated.
 */
function ensureAsyncContext (middleware) {
  return (req, res, next) => middleware(req, res, AsyncResource.bind(next))
}

Usage:

router.post(
  '/upload',
  // ... other plugins
  ensureAsyncContext(multerUpload.single('myUpload')),
  async (req, res, next) => {
    // ...
  },
);

victorlampp added a commit to victorlampp/multer that referenced this issue Apr 12, 2023
@GutoMartins19
Copy link

I just thought I'd share a cheap user-space workaround that my team came up with, in case it's helpful for anyone else.

const { AsyncResource } = require('node:async_hooks')

/**
 * Ensure that an express middleware respects async local storage context.
 *
 * Accepts an express middleware and returns a copy of that middleware with the
 * next() call explicitly bound to the current AsyncResource context.
 *
 * When working with native promises or async/await syntax, Node can
 * automatically forward any existing async resource to nested async or promise
 * invocations. However, when working with nested callback-passing patterns
 * (such as occurs in an express middleware stack) Node cannot know whether the
 * function being passed around should be bound to the current resource.
 *
 * Express may update their plugin architecture to resolve this in a future
 * version. Middleware developers can also update their implementations to
 * ensure backward-compatible support for AsyncLocalStorage. This function
 * allows us to work around the issue for current versions of express and
 * plugins that have not yet been updated.
 */
function ensureAsyncContext (middleware) {
  return (req, res, next) => middleware(req, res, AsyncResource.bind(next))
}

Usage:

router.post(
  '/upload',
  // ... other plugins
  ensureAsyncContext(multerUpload.single('myUpload')),
  async (req, res, next) => {
    // ...
  },
);

Thanks for sharing!

cdimascio pushed a commit to cdimascio/express-openapi-validator that referenced this issue Jan 25, 2024
related issue: expressjs/multer#814
Used the solution described in the above link to fix the issue

Co-authored-by: Alan Wang <alan@tacen.app>
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.

5 participants