lost context when use @fastify/multipart

[wvq]

Prerequisites

• I have written a descriptive issue title
• I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.15.0

Plugin version

4.2.0

Node.js version

18.14.0

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

11.3

Description

when register multipart after context, the router frequently lost context info

Steps to Reproduce

const Fastify = require('fastify').default
const MultipartSupport = require('@fastify/multipart')
const { requestContext } = require('@fastify/request-context')
const RequestContextSupport = require('@fastify/request-context')

const server = Fastify({ logger: true })


server.register(RequestContextSupport, {
  defaultStoreValues: {
    user: { id: 0 },
  },
})

server.addHook('onRequest', async (req, reply) => {
  req.requestContext.set('user', { id: 1 })
})

server.register(MultipartSupport, {
  addToBody: true,
})

server.post('/', async (req, reply) => {
  console.log(req.requestContext.get('user'))

  return { message: 'ok' }
})

server.listen({ host: '0.0.0.0', port: 3002 })

use postman to send any file to http://127.0.0.1/,
console.log(req.requestContext.get('user')) sometimes are undefined

Expected Behavior

No response

[climba03003]

The comment from source code already explain why it happen.
It is the limitation of EventEmitter async resources.

fastify-request-context/index.js

Lines 32 to 41 in e5a7220

	// Both of onRequest and preParsing are executed after the als.runWith call within the "proper" async context (AsyncResource implicitly created by ALS).
	// However, preValidation, preHandler and the route handler are executed as a part of req.emit('end') call which happens
	// in a different async context, as req/res may emit events in a different context.
	// Related to https://github.com/nodejs/node/issues/34430 and https://github.com/nodejs/node/issues/33723
	if (hook === 'onRequest' || hook === 'preParsing') {
	fastify.addHook('preValidation', (req, res, done) => {
	const asyncResource = req[asyncResourceSymbol]
	asyncResource.runInAsyncScope(done, req.raw)
	})
	}

It would be better to point it out in the readme.
Would you like to work on this?

[kibertoad]

@climba03003 Shouldn't the commented block help restore the correct context in that case? It was added to fix the context being lost when using body-parser, I wonder if some additional tweak is needed there to correctly handle multipart parsing hook as well.

[climba03003]

Shouldn't the commented block help restore the correct context in that case?

Yes, it also explain why the parser will mess up the context because busboy also an EventEmitter.

[wvq]

@climba03003 @kibertoad
Thanks for reply. But it still confused me, as it run on different async context, why sometimes output context correct?

[kibertoad]

I'll write a couple of tests, but I think that it should work correctly if you register request-context plugin after the multipart one, so that it gets a chance to rebind the context correctly

[wvq]

Yes, I register request-context after multipart to avoid this problem, it works fine.

In my situaction, I want:

1. get header properties then set user info to request-context.
2. in multipart onFile callback, store files in difference directory base on the user info
3. process other logic in some routes also need user info (has lost context problem)

Now, Call step 2 first, means the onFile callback need resolve step 1 logic;
and then call step 1 to set context, it a bad performance.

To avoid performance issue, I need to set property to req, and step 1 check if the property exists, the code is ugly.

[mcollina]

I think we should solve this in our multipart module by adding a AsyncResource at the right moment.

[mcollina]

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

[gustakasn0v]

@climba03003 @mcollina Now that #173 has been merged, does that mean this issue can be resolved? Or is there still work left to fix compatibility with @fastify/multipart?

Asking so I can at least raise a quick PR to add this gotcha to the readme

[Alt-er]

In the @fastify/request-context plugin implementation, I noticed this pattern:

asyncLocalStorage.run({ ...defaultStoreValues }, () => {
      const asyncResource =
        opts.createAsyncResource != null
          ? opts.createAsyncResource(req, requestContext)
          : new AsyncResource('fastify-request-context')
      req[asyncResourceSymbol] = asyncResource
      asyncResource.runInAsyncScope(done, req.raw)  // <-- Why not just done()?
    })

What's the practical difference between using asyncResource.runInAsyncScope(done, req.raw) and calling done() directly in this context?