-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
Fix context loss when async storage is instantiated early in the request lifecycle #12
Changes from 14 commits
2a9df9c
4e8b091
3da0d3b
7bce481
075a526
d405af2
d97cb01
48c23e7
c37fe5c
965f256
5e19b08
0be0b99
b7384f0
d859cfa
4cedd50
ecaea79
457f7ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,37 @@ | ||
const fp = require('fastify-plugin') | ||
const { als } = require('asynchronous-local-storage') | ||
const { AsyncResource } = require('async_hooks') | ||
|
||
const requestContext = { | ||
get: als.get, | ||
set: als.set, | ||
} | ||
|
||
/** | ||
* Monkey patches `.emit()` method of the given emitter, so | ||
* that all event listeners are run in scope of the provided | ||
* async resource. | ||
*/ | ||
const wrapEmitter = (emitter, asyncResource) => { | ||
const original = emitter.emit | ||
emitter.emit = function (type, ...args) { | ||
return asyncResource.runInAsyncScope(original, emitter, type, ...args) | ||
} | ||
} | ||
|
||
const wrapHttpEmitters = (req, res) => { | ||
const asyncResource = new AsyncResource('fastify-request-context') | ||
wrapEmitter(req, asyncResource) | ||
wrapEmitter(res, asyncResource) | ||
} | ||
|
||
function plugin(fastify, opts, next) { | ||
fastify.decorate('requestContext', requestContext) | ||
fastify.decorateRequest('requestContext', requestContext) | ||
|
||
fastify.addHook('onRequest', (req, res, done) => { | ||
fastify.addHook(opts.hook || 'onRequest', (req, res, done) => { | ||
als.runWith(() => { | ||
wrapHttpEmitters(req.raw, res.raw) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @puzpuzpuz In your example I see that you are using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIRC that property is not available in Fastify v2.x which my plugin requires. |
||
done() | ||
}, opts.defaultStoreValues) | ||
}) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
const fastify = require('fastify') | ||
const { fastifyRequestContextPlugin } = require('../../lib/requestContextPlugin') | ||
|
||
function initAppGet(endpoint) { | ||
const app = fastify({ logger: true }) | ||
app.register(fastifyRequestContextPlugin) | ||
|
||
app.get('/', endpoint) | ||
return app | ||
} | ||
|
||
function initAppPost(endpoint) { | ||
const app = fastify({ logger: true }) | ||
app.register(fastifyRequestContextPlugin) | ||
|
||
app.post('/', endpoint) | ||
|
||
return app | ||
} | ||
|
||
function initAppPostWithPrevalidation(endpoint) { | ||
const app = fastify({ logger: true }) | ||
app.register(fastifyRequestContextPlugin, { hook: 'preValidation' }) | ||
|
||
const preValidationFn = (req, reply, done) => { | ||
const requestId = Number.parseInt(req.body.requestId) | ||
req.requestContext.set('testKey', `testValue${requestId}`) | ||
done() | ||
} | ||
|
||
app.route({ | ||
url: '/', | ||
method: ['GET', 'POST'], | ||
preValidation: preValidationFn, | ||
handler: endpoint, | ||
}) | ||
|
||
return app | ||
} | ||
|
||
function initAppPostWithAllPlugins(endpoint, requestHook) { | ||
const app = fastify({ logger: true }) | ||
app.register(fastifyRequestContextPlugin, { hook: requestHook }) | ||
|
||
app.addHook('onRequest', (req, reply, done) => { | ||
req.requestContext.set('onRequest', 'dummy') | ||
done() | ||
}) | ||
|
||
app.addHook('preParsing', (req, reply, payload, done) => { | ||
req.requestContext.set('preParsing', 'dummy') | ||
done(null, payload) | ||
}) | ||
|
||
app.addHook('preValidation', (req, reply, done) => { | ||
const requestId = Number.parseInt(req.body.requestId) | ||
req.requestContext.set('preValidation', requestId) | ||
req.requestContext.set('testKey', `testValue${requestId}`) | ||
done() | ||
}) | ||
|
||
app.addHook('preHandler', (req, reply, done) => { | ||
const requestId = Number.parseInt(req.body.requestId) | ||
req.requestContext.set('preHandler', requestId) | ||
done() | ||
}) | ||
|
||
app.route({ | ||
url: '/', | ||
method: ['GET', 'POST'], | ||
handler: endpoint, | ||
}) | ||
|
||
return app | ||
} | ||
|
||
module.exports = { | ||
initAppPostWithAllPlugins, | ||
initAppPostWithPrevalidation, | ||
initAppPost, | ||
initAppGet, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,190 @@ | ||
const request = require('superagent') | ||
const { | ||
initAppPostWithPrevalidation, | ||
initAppPostWithAllPlugins, | ||
} = require('./internal/appInitializer') | ||
const { TestService } = require('./internal/testService') | ||
|
||
describe('requestContextPlugin E2E', () => { | ||
let app | ||
afterEach(() => { | ||
return app.close() | ||
}) | ||
|
||
it('correctly preserves values set in prevalidation phase within single POST request', () => { | ||
expect.assertions(2) | ||
|
||
let testService | ||
let responseCounter = 0 | ||
return new Promise((resolveResponsePromise) => { | ||
const promiseRequest2 = new Promise((resolveRequest2Promise) => { | ||
const promiseRequest1 = new Promise((resolveRequest1Promise) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please use async functions instead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure that would work as expected? In this case we want to able to have flexibility to control when each promise resolves specifically, and I don't think that is possible with async functions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh ok. |
||
const route = (req) => { | ||
const requestId = req.requestContext.get('testKey') | ||
|
||
function prepareReply() { | ||
return testService.processRequest(requestId.replace('testValue', '')).then(() => { | ||
const storedValue = req.requestContext.get('testKey') | ||
return Promise.resolve({ storedValue }) | ||
}) | ||
} | ||
|
||
// We don't want to read values until both requests wrote their values to see if there is a racing condition | ||
if (requestId === 'testValue1') { | ||
resolveRequest1Promise() | ||
return promiseRequest2.then(prepareReply) | ||
} | ||
|
||
if (requestId === 'testValue2') { | ||
resolveRequest2Promise() | ||
return promiseRequest1.then(prepareReply) | ||
} | ||
|
||
throw new Error(`Unexpected requestId: ${requestId}`) | ||
} | ||
|
||
app = initAppPostWithPrevalidation(route) | ||
app.listen(0).then(() => { | ||
testService = new TestService(app) | ||
const { address, port } = app.server.address() | ||
const url = `${address}:${port}` | ||
const response1Promise = request('POST', url) | ||
.send({ requestId: 1 }) | ||
.then((response) => { | ||
expect(response.body.storedValue).toBe('testValue1') | ||
responseCounter++ | ||
if (responseCounter === 2) { | ||
resolveResponsePromise() | ||
} | ||
}) | ||
|
||
const response2Promise = request('POST', url) | ||
.send({ requestId: 2 }) | ||
.then((response) => { | ||
expect(response.body.storedValue).toBe('testValue2') | ||
responseCounter++ | ||
if (responseCounter === 2) { | ||
resolveResponsePromise() | ||
} | ||
}) | ||
|
||
return Promise.all([response1Promise, response2Promise]) | ||
}) | ||
}) | ||
|
||
return promiseRequest1 | ||
}) | ||
|
||
return promiseRequest2 | ||
}) | ||
}) | ||
|
||
it('correctly preserves values set in multiple phases within single POST request', () => { | ||
expect.assertions(10) | ||
|
||
let testService | ||
let responseCounter = 0 | ||
return new Promise((resolveResponsePromise) => { | ||
const promiseRequest2 = new Promise((resolveRequest2Promise) => { | ||
const promiseRequest1 = new Promise((resolveRequest1Promise) => { | ||
const route = (req) => { | ||
const onRequestValue = req.requestContext.get('onRequest') | ||
const preParsingValue = req.requestContext.get('preParsing') | ||
const preValidationValue = req.requestContext.get('preValidation') | ||
const preHandlerValue = req.requestContext.get('preHandler') | ||
|
||
expect(onRequestValue).toBe(undefined) | ||
expect(preParsingValue).toBe(undefined) | ||
expect(preValidationValue).toEqual(expect.any(Number)) | ||
expect(preHandlerValue).toEqual(expect.any(Number)) | ||
|
||
const requestId = `testValue${preHandlerValue}` | ||
|
||
function prepareReply() { | ||
return testService.processRequest(requestId.replace('testValue', '')).then(() => { | ||
const storedValue = req.requestContext.get('preValidation') | ||
return Promise.resolve({ storedValue: `testValue${storedValue}` }) | ||
}) | ||
} | ||
|
||
// We don't want to read values until both requests wrote their values to see if there is a racing condition | ||
if (requestId === 'testValue1') { | ||
resolveRequest1Promise() | ||
return promiseRequest2.then(prepareReply) | ||
} | ||
|
||
if (requestId === 'testValue2') { | ||
resolveRequest2Promise() | ||
return promiseRequest1.then(prepareReply) | ||
} | ||
|
||
throw new Error(`Unexpected requestId: ${requestId}`) | ||
} | ||
|
||
app = initAppPostWithAllPlugins(route, 'preValidation') | ||
|
||
app.listen(0).then(() => { | ||
testService = new TestService(app) | ||
const { address, port } = app.server.address() | ||
const url = `${address}:${port}` | ||
const response1Promise = request('POST', url) | ||
.send({ requestId: 1 }) | ||
.then((response) => { | ||
expect(response.body.storedValue).toBe('testValue1') | ||
responseCounter++ | ||
if (responseCounter === 2) { | ||
resolveResponsePromise() | ||
} | ||
}) | ||
|
||
const response2Promise = request('POST', url) | ||
.send({ requestId: 2 }) | ||
.then((response) => { | ||
expect(response.body.storedValue).toBe('testValue2') | ||
responseCounter++ | ||
if (responseCounter === 2) { | ||
resolveResponsePromise() | ||
} | ||
}) | ||
|
||
return Promise.all([response1Promise, response2Promise]) | ||
}) | ||
}) | ||
|
||
return promiseRequest1 | ||
}) | ||
|
||
return promiseRequest2 | ||
}) | ||
}) | ||
|
||
it('does not lose request context after body parsing', () => { | ||
expect.assertions(5) | ||
const route = (req) => { | ||
const onRequestValue = req.requestContext.get('onRequest') | ||
const preParsingValue = req.requestContext.get('preParsing') | ||
const preValidationValue = req.requestContext.get('preValidation') | ||
const preHandlerValue = req.requestContext.get('preHandler') | ||
|
||
expect(onRequestValue).toBe('dummy') | ||
expect(preParsingValue).toBe('dummy') | ||
expect(preValidationValue).toEqual(expect.any(Number)) | ||
expect(preHandlerValue).toEqual(expect.any(Number)) | ||
|
||
const requestId = `testValue${preHandlerValue}` | ||
return Promise.resolve({ storedValue: requestId }) | ||
} | ||
|
||
app = initAppPostWithAllPlugins(route, 'onRequest') | ||
|
||
return app.listen(0).then(() => { | ||
const { address, port } = app.server.address() | ||
const url = `${address}:${port}` | ||
return request('POST', url) | ||
.send({ requestId: 1 }) | ||
.then((response) => { | ||
expect(response.body.storedValue).toBe('testValue1') | ||
}) | ||
}) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might want to run this on multiple hooks as well, maybe it should be an array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, but what is the use-case for that? Why would you want to reinitialize storage on multiple steps, if just the earliest one should suffice? Also I'm thinking of switching default to
preValidation
, as it seems to be logical to init it as early as possible. Thoughts?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onRequest
happens beforepreValidation
.I don't think it should be reinitialized, but probably something must be done to retain the context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I digged deeper, and yes, we are definitely losing the context after payload parsing is triggered (as in we retain storage betwen
onRequest
andpreParsing
, but lose betweenpreParsing
andpreValidation
), which actually seems to be a problem that was around a while and was breaking CLS in the past as well. Still investigating what the options are.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a variation of nodejs/node#34401
What are we using for body parsing in fastify? some kind of custom in-house solution? is there some sort of pooling involved there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the request object is available in all hooks, then yes, it should be possible to initialize an
AsyncResource
early enough, store it inreq
and then restore it where necessary and, may be, wrap next calls withasyncResource.runInAsyncScope
. This approach also requires proper integration between ALS and theAsyncResource
, but that part is trivial.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's possible, the core request is available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@puzpuzpuz @mcollina What am I doing wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kibertoad you should move
AsyncResource
ctor call inside of theals.runWith
call. This way it'll be associated with the same store object (I mean theMap
object created by theals
wrapper library).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@puzpuzpuz Thanks!
@mcollina I believe PR is ready for the final rereview.