-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(koa): defer AsyncLocalStorage creation for V8 startup snapshot support #5851
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
Changes from all commits
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 |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| import assert from 'node:assert/strict'; | ||
| import { AsyncLocalStorage } from 'node:async_hooks'; | ||
| import v8 from 'node:v8'; | ||
|
|
||
| import { request } from '@eggjs/supertest'; | ||
| import { afterEach, describe, it } from 'vitest'; | ||
|
|
||
| import Koa from '../../src/index.ts'; | ||
|
|
||
| describe('v8 startup snapshot', () => { | ||
| const originalStartupSnapshot = v8.startupSnapshot; | ||
|
|
||
| afterEach(() => { | ||
| (v8 as Record<string, unknown>).startupSnapshot = originalStartupSnapshot; | ||
| }); | ||
|
|
||
| it('should defer AsyncLocalStorage creation when building snapshot', () => { | ||
| let deserializeCallback: { cb: (data: unknown) => void; data: unknown } | undefined; | ||
| (v8 as Record<string, unknown>).startupSnapshot = { | ||
|
Contributor
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. Mutating properties of the
Contributor
Author
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. Thanks for the suggestion. The direct mutation via |
||
| isBuildingSnapshot: () => true, | ||
| addDeserializeCallback: (cb: (data: unknown) => void, data: unknown) => { | ||
| deserializeCallback = { cb, data }; | ||
| }, | ||
| }; | ||
|
|
||
| const app = new Koa(); | ||
| assert.strictEqual(app.ctxStorage, null); | ||
| assert.ok(deserializeCallback, 'deserialize callback should be registered'); | ||
|
|
||
| // simulate snapshot deserialization | ||
| deserializeCallback.cb(deserializeCallback.data); | ||
| assert.ok(app.ctxStorage! instanceof AsyncLocalStorage); | ||
| }); | ||
|
|
||
| it('should return undefined for currentContext when ctxStorage is null', () => { | ||
| (v8 as Record<string, unknown>).startupSnapshot = { | ||
| isBuildingSnapshot: () => true, | ||
| addDeserializeCallback: () => {}, | ||
| }; | ||
|
|
||
| const app = new Koa(); | ||
| assert.strictEqual(app.ctxStorage, null); | ||
| assert.strictEqual(app.currentContext, undefined); | ||
| }); | ||
|
|
||
| it('should work normally after deserialization', async () => { | ||
| let deserializeCallback: { cb: (data: unknown) => void; data: unknown } | undefined; | ||
| (v8 as Record<string, unknown>).startupSnapshot = { | ||
| isBuildingSnapshot: () => true, | ||
| addDeserializeCallback: (cb: (data: unknown) => void, data: unknown) => { | ||
| deserializeCallback = { cb, data }; | ||
| }, | ||
| }; | ||
|
|
||
| const app = new Koa(); | ||
|
|
||
| // simulate snapshot deserialization | ||
| deserializeCallback!.cb(deserializeCallback!.data); | ||
|
|
||
| app.use(async (ctx) => { | ||
| assert.equal(ctx, app.currentContext); | ||
| ctx.body = 'ok'; | ||
| }); | ||
|
|
||
| await request(app.callback()).get('/').expect('ok'); | ||
| assert.strictEqual(app.currentContext, undefined); | ||
| }); | ||
|
|
||
| it('should not defer when not building snapshot', () => { | ||
| (v8 as Record<string, unknown>).startupSnapshot = { | ||
| isBuildingSnapshot: () => false, | ||
| }; | ||
|
|
||
| const app = new Koa(); | ||
| assert.ok(app.ctxStorage! instanceof AsyncLocalStorage); | ||
| }); | ||
|
|
||
| it('should handle callback without ctxStorage during snapshot build', async () => { | ||
| (v8 as Record<string, unknown>).startupSnapshot = { | ||
| isBuildingSnapshot: () => true, | ||
| addDeserializeCallback: () => {}, | ||
| }; | ||
|
|
||
| const app = new Koa(); | ||
| assert.strictEqual(app.ctxStorage, null); | ||
|
|
||
| app.use(async (ctx) => { | ||
| assert.strictEqual(app.currentContext, undefined); | ||
| ctx.body = 'ok'; | ||
| }); | ||
|
|
||
| await request(app.callback()).get('/').expect('ok'); | ||
| }); | ||
| }); | ||
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.
While
v8.startupSnapshotis checked on line 92, it is safer to use optional chaining or verify the existence ofaddDeserializeCallbackbefore calling it. This API is environment-dependent and may not be fully present in all Node.js versions or environments where the code might run.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.
Won't fix. Line 92 already guards with
v8.startupSnapshot?.isBuildingSnapshot?.()— if this returnstrue, the full snapshot API (includingaddDeserializeCallback) is guaranteed to be present per the Node.js docs. Adding?.here would silently swallow errors if the API were somehow incomplete, which would be a worse failure mode:ctxStoragewould staynullforever with no error, causing subtle runtime issues. Failing fast is the safer behavior.