-
Notifications
You must be signed in to change notification settings - Fork 267
fix(NODE-7575): reset ObjectId state when building startup snapshot #884
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
base: main
Are you sure you want to change the base?
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 |
|---|---|---|
|
|
@@ -4,9 +4,6 @@ import { type InspectFn, defaultInspect } from './parser/utils'; | |
| import { ByteUtils } from './utils/byte_utils'; | ||
| import { NumberUtils } from './utils/number_utils'; | ||
|
|
||
| // Unique sequence for the current process (initialized on first use) | ||
| let PROCESS_UNIQUE: Uint8Array | null = null; | ||
|
|
||
| /** ObjectId hexString cache @internal */ | ||
| const __idCache = new WeakMap(); // TODO(NODE-6549): convert this to #__id private field when target updated to ES2022 | ||
|
|
||
|
|
@@ -33,7 +30,28 @@ export class ObjectId extends BSONValue { | |
| } | ||
|
|
||
| /** @internal */ | ||
| private static index = Math.floor(Math.random() * 0xffffff); | ||
| private static index = 0; | ||
|
|
||
| /** Unique sequence for the current process (initialized on first use) | ||
| * @internal | ||
| */ | ||
| private static PROCESS_UNIQUE: Uint8Array | null = null; | ||
|
|
||
| /** @internal */ | ||
| private static resetState = (): void => { | ||
| this.index = Math.floor(Math.random() * 0x1000000); | ||
|
addaleax marked this conversation as resolved.
|
||
| this.PROCESS_UNIQUE = null; | ||
| }; | ||
|
|
||
| static { | ||
| this.resetState(); | ||
| // https://nodejs.org/api/v8.html#startup-snapshot-api | ||
| // @ts-expect-error Node.js types not present since this is an optional API | ||
| const { startupSnapshot } = globalThis?.process?.getBuiltinModule('v8') ?? {}; | ||
|
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. Accessing the API this way was recommended here: nodejs/node#63138 (comment) I would agree that this seems like a reasonable compromise |
||
| if (startupSnapshot?.isBuildingSnapshot()) { | ||
| startupSnapshot?.addDeserializeCallback(this.resetState); | ||
| } | ||
| } | ||
|
|
||
| static cacheHexString: boolean; | ||
|
|
||
|
|
@@ -178,7 +196,7 @@ export class ObjectId extends BSONValue { | |
| * @internal | ||
| */ | ||
| private static getInc(): number { | ||
| return (ObjectId.index = (ObjectId.index + 1) % 0xffffff); | ||
| return (ObjectId.index = (ObjectId.index + 1) % 0x1000000); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -198,9 +216,7 @@ export class ObjectId extends BSONValue { | |
| NumberUtils.setInt32BE(buffer, 0, time); | ||
|
|
||
| // set PROCESS_UNIQUE if yet not initialized | ||
| if (PROCESS_UNIQUE === null) { | ||
| PROCESS_UNIQUE = ByteUtils.randomBytes(5); | ||
| } | ||
| const PROCESS_UNIQUE = (this.PROCESS_UNIQUE ??= ByteUtils.randomBytes(5)); | ||
|
|
||
| // 5-byte process unique | ||
| buffer[4] = PROCESS_UNIQUE[0]; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| import * as child_process from 'node:child_process'; | ||
| import * as os from 'node:os'; | ||
| import * as path from 'node:path'; | ||
| import * as fs from 'node:fs/promises'; | ||
| import { promisify } from 'node:util'; | ||
| import { expect } from 'chai'; | ||
|
|
||
| describe('snapshot support', () => { | ||
| let tmpdir: string; | ||
|
|
||
| beforeEach(async () => { | ||
| tmpdir = await fs.mkdtemp(path.join(os.tmpdir(), 'js-bson-snapshot-test-')); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| await fs.rm(tmpdir, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| // Regression test for https://jira.mongodb.org/browse/MONGOSH-3277 | ||
| it('should reset relevant state when using startup snapshot', async () => { | ||
| // Build a startup snapshot including the BSON library and an example ObjectId | ||
| const bsonBundleSource = path.join(__dirname, '..', '..', 'lib', 'bson.bundle.js'); | ||
| await fs.writeFile( | ||
| path.join(tmpdir, 'snapshot_main.cjs'), | ||
| ` | ||
| ${await fs.readFile(bsonBundleSource, 'utf8')} | ||
| const { startupSnapshot } = require('v8'); | ||
| globalThis.pid = new BSON.ObjectId(); | ||
| startupSnapshot.setDeserializeMainFunction(() => { | ||
| console.log(globalThis.pid.toHexString()); | ||
| console.log(new BSON.ObjectId().toHexString()); | ||
| }); | ||
| ` | ||
| ); | ||
| await promisify(child_process.execFile)( | ||
| process.execPath, | ||
| ['--snapshot-blob', 'snapshot.blob', '--build-snapshot', 'snapshot_main.cjs'], | ||
| { | ||
| cwd: tmpdir, | ||
| encoding: 'utf8' | ||
| } | ||
| ); | ||
|
|
||
| // Run the resulting snapshot twice to compare | ||
| const stdout1 = ( | ||
| await promisify(child_process.execFile)( | ||
| process.execPath, | ||
| ['--snapshot-blob', 'snapshot.blob'], | ||
| { | ||
| cwd: tmpdir, | ||
| encoding: 'utf8' | ||
| } | ||
| ) | ||
| ).stdout | ||
| .trim() | ||
| .split('\n'); | ||
| const stdout2 = ( | ||
| await promisify(child_process.execFile)( | ||
| process.execPath, | ||
| ['--snapshot-blob', 'snapshot.blob'], | ||
| { | ||
| cwd: tmpdir, | ||
| encoding: 'utf8' | ||
| } | ||
| ) | ||
| ).stdout | ||
| .trim() | ||
| .split('\n'); | ||
|
|
||
| // Each process should print two values | ||
| expect(stdout1).to.have.lengthOf(2); | ||
| expect(stdout2).to.have.lengthOf(2); | ||
|
|
||
| // The in-snapshot ObjectId is persisted | ||
| expect(stdout1[0]).to.equal(stdout2[0]); | ||
|
|
||
| // We get different per-process values (counter and process unique) | ||
| // created after deserialization | ||
| const [oid1, oid2] = [stdout1[1], stdout2[1]].map( | ||
| oid => oid.match(/^(?<ts>\w{8})(?<uniq>\w{10})(?<counter>\w{6})$/)!.groups! | ||
| ); | ||
|
|
||
| // Less than 20 seconds between timestamps should be plenty of leeway | ||
| expect(Math.abs(parseInt(oid2.ts, 16) - parseInt(oid1.ts, 16))).to.be.lessThan(20); | ||
| // Distinct process unique values | ||
| expect(oid1.uniq).to.not.equal(oid2.uniq); | ||
| // Distinct counter values | ||
| expect(oid1.counter).to.not.equal(oid2.counter); | ||
|
Comment on lines
+83
to
+88
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. Yeah, this is one in 16 million CI runs that fails, I can live with that |
||
| }); | ||
| }); | ||
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.
Drive-by fix:
Math.random()never returns1, and0xffffffshould be a valid counter value