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

src: fail CommonEnvironmentSetup if environment bootstrapping failed #46797

Closed
wants to merge 1 commit into from

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Feb 23, 2023

If the Environment is failed to bootstrap at development time (e.g. syntax errors or uncaught exceptions in the JavaScript bootstrapping codes), the CommonEnvironmentSetup::CreateForSnapshotting should return a nullptr instead of a CommonEnvironmentSetup with a nullptr env member, which would lead to invalid access.

Unfortunately, I'm not aware of a method to run an unhappy path in bootstrapping to add a test case for this behavior.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Feb 23, 2023
@addaleax
Copy link
Member

Mh this might be a duplicate of #46533? The approach there is a bit more complete because it passes along the actual error message (but also does a quite a bit of work in order to be able to do so)

@legendecas
Copy link
Member Author

Ah, thanks. Closing as duplicated.

@legendecas legendecas closed this Feb 23, 2023
@addaleax addaleax added the duplicate Issues and PRs that are duplicates of other issues or PRs. label Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. duplicate Issues and PRs that are duplicates of other issues or PRs. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants