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.

Sign up for GitHub

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

Jump to bottom

src: allow blobs in addition to FILE*s in embedder snapshot API #46491

Merged
merged 2 commits into from Feb 6, 2023
Merged

src: allow blobs in addition to FILE*s in embedder snapshot API #46491

merged 2 commits into from Feb 6, 2023

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Feb 4, 2023

This is a shared follow-up to 06bb6b4 and a466fea now that both have been merged.

Refs: #45888
Refs: #46463

@addaleax addaleax added c++ Issues and PRs that require attention from people who are familiar with C++. semver-minor PRs that contain new features and should be released in the next minor version. embedding Issues and PRs related to embedding Node.js in another project. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 4, 2023
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 4, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/49355/

joyeecheung
joyeecheung reviewed Feb 4, 2023
View reviewed changes
src/node_snapshotable.cc Show resolved Hide resolved
src/node_snapshotable.cc Outdated Show resolved Hide resolved
@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/49358/

joyeecheung
joyeecheung approved these changes Feb 4, 2023
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: “in addition to” instead of “instead of” in the commit message?

@addaleax addaleax changed the title src: allow blobs instead of FILE*s in embedder snapshot API src: allow blobs in addition to FILE*s in embedder snapshot API Feb 4, 2023
@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2023
@addaleax addaleax removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2023
@addaleax
Copy link
Member Author

addaleax commented Feb 6, 2023

Rebased to resolve trivial conflict with #46495

@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 6, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 6, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/49446/

tniessen
tniessen reviewed Feb 6, 2023
View reviewed changes
test/embedding/embedtest.cc Outdated
Comment on lines 143 to 152
FILE* fp = fopen((snapshot_arg_it + 1)->c_str(), "w");
assert(fp != nullptr);
snapshot->ToFile(fp);
if (snapshot_as_file_it != args.end()) {
snapshot->ToFile(fp);
} else {
const std::vector<char> vec = snapshot->ToBlob();
size_t written = fwrite(vec.data(), vec.size(), 1, fp);
assert(written == 1);
}
fclose(fp);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be entirely wrong about this, but my understanding of fwrite so far has been that its return value only indicates how many objects have been written to the (buffered) stream, regardless of whether they've been written to the underlying physical storage. Isn't it necessary to check the result of fflush() or fclose() to ensure that any buffered data is written to the storage medium?

Copy link
Member Author

@addaleax addaleax Feb 6, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you’re right, but practically speaking I don’t think this is something that we really need to worry about a lot here in the test. It probably doesn’t hurt to add a check though, so: Done in 0d928d7 👍

Wouldn’t that apply to #46497 as well then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry, I didn't realize I was referring to a test file. I think it's also true for non-test occurrences such as the same call to ToFile() in node.cc. (I can't seem to get the link to work, sorry).

Maybe add CHECK_EQ(fflush(out), 0) to ToFile() since it already aborts on certain I/O errors, so that the caller does not have to worry about it? Then it should be reasonably safe to ignore the return value of fclose() 🙂

Wouldn’t that apply to #46497 as well then?

I didn't look at what exactly FromBlob() is doing for that PR and just moved the fclose() call around, but it seems to me that FromBlob() cannot run into similar problems because it first determines the size of the entire file and then checks whether it successfully read the number of bytes. Buffering would happen after actual I/O, so if FromBlob() succeeds, no relevant I/O error has occurred. (When writing, buffering happens before actual I/O.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that makes sense to me. I’ll follow up with a PR to do that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR: #46531

@tniessen
Copy link
Member

tniessen commented Feb 6, 2023

@addaleax Sorry, I realized too late that #46497 also conflicts.

@addaleax
Copy link
Member Author

addaleax commented Feb 6, 2023

@tniessen All good! The conflict only came up when renaming SnapshotData::FromBlob to SnapshotData::FromFile which I did after the PR here was already opened, so I missed that myself as well 🙂

@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 6, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 6, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/49450/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/49454/

@addaleax addaleax added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Feb 6, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 6, 2023
@nodejs-github-bot nodejs-github-bot merged commit d592630 into nodejs:main Feb 6, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in d592630

@addaleax addaleax deleted the embedder-snapshot-blob branch February 6, 2023 19:58
@addaleax addaleax mentioned this pull request Feb 6, 2023
Merged
addaleax added a commit to addaleax/node that referenced this pull request Feb 6, 2023
@addaleax
src: add fflush() to SnapshotData::ToFile()
e37f0aa 
Refs: nodejs#46491 (comment)
@github-actions github-actions bot mentioned this pull request Feb 7, 2023
Open
18 tasks
nodejs-github-bot pushed a commit that referenced this pull request Feb 8, 2023
@addaleax
src: add fflush() to SnapshotData::ToFile()
5092346 
Refs: #46491 (comment)
PR-URL: #46531
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
@addaleax @MylesBorins
src: allow blobs in addition to FILE*s in embedder snapshot API
c34bac2 
This is a shared follow-up to 06bb6b4 and a466fea
now that both have been merged.

PR-URL: #46491
Refs: #45888
Refs: #46463
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
@addaleax @MylesBorins
src: add fflush() to SnapshotData::ToFile()
31fb2e2 
Refs: #46491 (comment)
PR-URL: #46531
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins added a commit that referenced this pull request Feb 19, 2023
@MylesBorins
2023-02-21, Version 19.7.0 (Current)
eb53c0f 
Notable changes:

deps:
  * upgrade npm to 9.5.0 (npm team) #46673
  * add ada as a dependency (Yagiz Nizipli) #46410
doc:
  * add debadree25 to collaborators (Debadree Chatterjee) #46716
  * add deokjinkim to collaborators (Deokjin Kim) #46444
doc,lib,src,test:
  * rename --test-coverage (Colin Ihrig) #46017
lib:
  * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494
src:
  * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038
  * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583
  * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491
  * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888
  * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368
stream:
  * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273
test_runner:
  * add initial code coverage support (Colin Ihrig) #46017
url:
  * replace url-parser with ada (Yagiz Nizipli) #46410

PR-URL: TODO
@MylesBorins MylesBorins mentioned this pull request Feb 19, 2023
Merged
MylesBorins added a commit that referenced this pull request Feb 20, 2023
@MylesBorins
2023-02-21, Version 19.7.0 (Current)
051c9c3 
Notable changes:

deps:
  * upgrade npm to 9.5.0 (npm team) #46673
  * add ada as a dependency (Yagiz Nizipli) #46410
doc:
  * add debadree25 to collaborators (Debadree Chatterjee) #46716
  * add deokjinkim to collaborators (Deokjin Kim) #46444
doc,lib,src,test:
  * rename --test-coverage (Colin Ihrig) #46017
lib:
  * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494
src:
  * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038
  * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583
  * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491
  * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888
  * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368
stream:
  * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273
test_runner:
  * add initial code coverage support (Colin Ihrig) #46017
url:
  * replace url-parser with ada (Yagiz Nizipli) #46410

PR-URL: #46725
MylesBorins added a commit to MylesBorins/node that referenced this pull request Feb 20, 2023
@MylesBorins
2023-02-21, Version 19.7.0 (Current)
397dc01 
Notable changes:

deps:
  * upgrade npm to 9.5.0 (npm team) nodejs#46673
  * add ada as a dependency (Yagiz Nizipli) nodejs#46410
doc:
  * add debadree25 to collaborators (Debadree Chatterjee) nodejs#46716
  * add deokjinkim to collaborators (Deokjin Kim) nodejs#46444
doc,lib,src,test:
  * rename --test-coverage (Colin Ihrig) nodejs#46017
lib:
  * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) nodejs#46494
src:
  * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) nodejs#45038
  * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) nodejs#46583
  * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) nodejs#46491
  * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) nodejs#45888
  * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) nodejs#45888
  * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) nodejs#45888
  * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) nodejs#46368
stream:
  * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) nodejs#46273
test_runner:
  * add initial code coverage support (Colin Ihrig) nodejs#46017
url:
  * replace url-parser with ada (Yagiz Nizipli) nodejs#46410

PR-URL: nodejs#46725
MylesBorins added a commit that referenced this pull request Feb 20, 2023
@MylesBorins
2023-02-21, Version 19.7.0 (Current)
3ee97ee 
Notable changes:

deps:
  * upgrade npm to 9.5.0 (npm team) #46673
  * add ada as a dependency (Yagiz Nizipli) #46410
doc:
  * add debadree25 to collaborators (Debadree Chatterjee) #46716
  * add deokjinkim to collaborators (Deokjin Kim) #46444
doc,lib,src,test:
  * rename --test-coverage (Colin Ihrig) #46017
lib:
  * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494
src:
  * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038
  * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583
  * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491
  * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888
  * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368
stream:
  * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273
test_runner:
  * add initial code coverage support (Colin Ihrig) #46017
url:
  * replace url-parser with ada (Yagiz Nizipli) #46410

PR-URL: #46725
MylesBorins added a commit that referenced this pull request Feb 20, 2023
@MylesBorins
2023-02-21, Version 19.7.0 (Current)
a7dbed7 
Notable changes:

deps:
  * upgrade npm to 9.5.0 (npm team) #46673
  * add ada as a dependency (Yagiz Nizipli) #46410
doc:
  * add debadree25 to collaborators (Debadree Chatterjee) #46716
  * add deokjinkim to collaborators (Deokjin Kim) #46444
doc,lib,src,test:
  * rename --test-coverage (Colin Ihrig) #46017
lib:
  * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494
src:
  * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038
  * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583
  * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491
  * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888
  * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368
stream:
  * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273
test_runner:
  * add initial code coverage support (Colin Ihrig) #46017
url:
  * replace url-parser with ada (Yagiz Nizipli) #46410

PR-URL: #46725
MylesBorins added a commit that referenced this pull request Feb 20, 2023
@MylesBorins
2023-02-21, Version 19.7.0 (Current)
00a428e 
Notable changes:

deps:
  * upgrade npm to 9.5.0 (npm team) #46673
  * add ada as a dependency (Yagiz Nizipli) #46410
doc:
  * add debadree25 to collaborators (Debadree Chatterjee) #46716
  * add deokjinkim to collaborators (Deokjin Kim) #46444
doc,lib,src,test:
  * rename --test-coverage (Colin Ihrig) #46017
lib:
  * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494
src:
  * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038
  * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583
  * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491
  * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888
  * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368
stream:
  * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273
test_runner:
  * add initial code coverage support (Colin Ihrig) #46017
url:
  * replace url-parser with ada (Yagiz Nizipli) #46410

PR-URL: #46725
MylesBorins added a commit that referenced this pull request Feb 21, 2023
@MylesBorins
2023-02-21, Version 19.7.0 (Current)
89322ae 
Notable changes:

deps:
  * upgrade npm to 9.5.0 (npm team) #46673
  * add ada as a dependency (Yagiz Nizipli) #46410
doc:
  * add debadree25 to collaborators (Debadree Chatterjee) #46716
  * add deokjinkim to collaborators (Deokjin Kim) #46444
doc,lib,src,test:
  * rename --test-coverage (Colin Ihrig) #46017
lib:
  * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494
src:
  * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038
  * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583
  * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491
  * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888
  * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368
stream:
  * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273
test_runner:
  * add initial code coverage support (Colin Ihrig) #46017
url:
  * replace url-parser with ada (Yagiz Nizipli) #46410

PR-URL: #46725
@danielleadams
Copy link
Member

@addaleax do you mind backporting this to v18.x? This has a handful of merge conflicts when trying to land. Thank you.

@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Apr 2, 2023
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
@addaleax @danielleadams
src: add fflush() to SnapshotData::ToFile()
9567436 
Refs: #46491 (comment)
PR-URL: #46531
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@tniessen tniessen tniessen approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. embedding Issues and PRs related to embedding Node.js in another project. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@addaleax @nodejs-github-bot @tniessen @danielleadams @jasnell @joyeecheung @RaisinTen