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

lib: account for cwd access from snapshot serialization cb #51901

Conversation

addaleax
Copy link
Member

Functions registered with addSerializeCallback() can access and call process.cwd(). b7d836e accounted for the fact that it is necessary to reset the cwd cache after the snapshot builder script has run, but did not account for possible accesses from serialization callbacks. To properly account for these, add a deserialization callback as well.

As a related drive-by fix, also mention the execution order of callbacks in the documentation.

Refs: #49684

Functions registered with `addSerializeCallback()` can access and call
`process.cwd()`. b7d836e accounted for the fact that it is
necessary to reset the cwd cache after the snapshot builder script has
run, but did not account for possible accesses from serialization
callbacks. To properly account for these, add a deserialization
callback as well.

As a related drive-by fix, also mention the execution order of
callbacks in the documentation.

Refs: nodejs#49684
@addaleax addaleax added the snapshot Issues and PRs related to the startup snapshot label Feb 27, 2024
@addaleax addaleax requested a review from joyeecheung February 27, 2024 20:50
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Feb 27, 2024
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2024
@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 28, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 29, 2024
@nodejs-github-bot nodejs-github-bot merged commit 40565e9 into nodejs:main Feb 29, 2024
65 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 40565e9

marco-ippolito pushed a commit that referenced this pull request Mar 1, 2024
Functions registered with `addSerializeCallback()` can access and call
`process.cwd()`. b7d836e accounted for the fact that it is
necessary to reset the cwd cache after the snapshot builder script has
run, but did not account for possible accesses from serialization
callbacks. To properly account for these, add a deserialization
callback as well.

As a related drive-by fix, also mention the execution order of
callbacks in the documentation.

Refs: #49684
PR-URL: #51901
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
@marco-ippolito marco-ippolito mentioned this pull request Mar 1, 2024
@addaleax addaleax deleted the snapshot-cwd-accessed-from-inside-serialize-callback branch March 1, 2024 13:10
richardlau pushed a commit that referenced this pull request Mar 25, 2024
Functions registered with `addSerializeCallback()` can access and call
`process.cwd()`. b7d836e accounted for the fact that it is
necessary to reset the cwd cache after the snapshot builder script has
run, but did not account for possible accesses from serialization
callbacks. To properly account for these, add a deserialization
callback as well.

As a related drive-by fix, also mention the execution order of
callbacks in the documentation.

Refs: #49684
PR-URL: #51901
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
Functions registered with `addSerializeCallback()` can access and call
`process.cwd()`. b7d836e accounted for the fact that it is
necessary to reset the cwd cache after the snapshot builder script has
run, but did not account for possible accesses from serialization
callbacks. To properly account for these, add a deserialization
callback as well.

As a related drive-by fix, also mention the execution order of
callbacks in the documentation.

Refs: #49684
PR-URL: #51901
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
@richardlau richardlau mentioned this pull request Mar 25, 2024
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
Functions registered with `addSerializeCallback()` can access and call
`process.cwd()`. b7d836e accounted for the fact that it is
necessary to reset the cwd cache after the snapshot builder script has
run, but did not account for possible accesses from serialization
callbacks. To properly account for these, add a deserialization
callback as well.

As a related drive-by fix, also mention the execution order of
callbacks in the documentation.

Refs: nodejs#49684
PR-URL: nodejs#51901
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. snapshot Issues and PRs related to the startup snapshot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants