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

    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);


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?

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?

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.)

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

tniessen · 2023-02-06T15:27:24Z

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

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

addaleax · 2023-02-06T16:00:46Z

@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 🙂

nodejs-github-bot · 2023-02-06T16:19:05Z

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

nodejs-github-bot · 2023-02-06T19:14:01Z

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

nodejs-github-bot · 2023-02-06T19:57:21Z

Landed in d592630

Refs: nodejs#46491 (comment)

Refs: #46491 (comment) PR-URL: #46531 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

Refs: #46491 (comment) PR-URL: #46531 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

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

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

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

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 · 2023-04-02T21:41:13Z

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

Refs: #46491 (comment) PR-URL: #46531 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 4, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2023

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 added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2023

joyeecheung approved these changes Feb 4, 2023

View reviewed changes

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 added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2023

addaleax force-pushed the embedder-snapshot-blob branch from 5fa6dec to 6071ff5 Compare February 4, 2023 14:59

addaleax removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2023

jasnell approved these changes Feb 4, 2023

View reviewed changes

RaisinTen approved these changes Feb 6, 2023

View reviewed changes

addaleax force-pushed the embedder-snapshot-blob branch from 6071ff5 to 5b0c5a5 Compare February 6, 2023 15:01

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 bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 6, 2023

tniessen reviewed Feb 6, 2023

View reviewed changes

addaleax added 2 commits February 6, 2023 16:59

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

56170f5

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

fixup! src: allow blobs in addition to FILE*s in embedder snapshot API

0d928d7

addaleax force-pushed the embedder-snapshot-blob branch from 5b0c5a5 to 0d928d7 Compare February 6, 2023 16:01

addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 6, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 6, 2023

tniessen approved these changes Feb 6, 2023

View reviewed changes

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 removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 6, 2023

nodejs-github-bot merged commit d592630 into nodejs:main Feb 6, 2023

addaleax deleted the embedder-snapshot-blob branch February 6, 2023 19:58

addaleax mentioned this pull request Feb 6, 2023

src: add fflush() to SnapshotData::ToFile() #46531

Merged

addaleax added a commit to addaleax/node that referenced this pull request Feb 6, 2023

src: add fflush() to SnapshotData::ToFile()

e37f0aa

Refs: nodejs#46491 (comment)

github-actions bot mentioned this pull request Feb 7, 2023

CI Reliability 2023-02-07 nodejs/reliability#498

Open

18 tasks

MylesBorins mentioned this pull request Feb 19, 2023

v19.7.0 proposal #46725

Merged

danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Apr 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

addaleax commented Feb 4, 2023

nodejs-github-bot commented Feb 4, 2023

nodejs-github-bot commented Feb 4, 2023

nodejs-github-bot commented Feb 4, 2023

joyeecheung left a comment

addaleax commented Feb 6, 2023

nodejs-github-bot commented Feb 6, 2023

tniessen Feb 6, 2023

addaleax Feb 6, 2023 •

edited

Loading

tniessen Feb 6, 2023

addaleax Feb 6, 2023

addaleax Feb 6, 2023

tniessen commented Feb 6, 2023

addaleax commented Feb 6, 2023

nodejs-github-bot commented Feb 6, 2023

nodejs-github-bot commented Feb 6, 2023

nodejs-github-bot commented Feb 6, 2023

danielleadams commented Apr 2, 2023

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

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

Conversation

addaleax commented Feb 4, 2023

nodejs-github-bot commented Feb 4, 2023

nodejs-github-bot commented Feb 4, 2023

nodejs-github-bot commented Feb 4, 2023

joyeecheung left a comment

Choose a reason for hiding this comment

addaleax commented Feb 6, 2023

nodejs-github-bot commented Feb 6, 2023

tniessen Feb 6, 2023

Choose a reason for hiding this comment

addaleax Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

tniessen Feb 6, 2023

Choose a reason for hiding this comment

addaleax Feb 6, 2023

Choose a reason for hiding this comment

addaleax Feb 6, 2023

Choose a reason for hiding this comment

tniessen commented Feb 6, 2023

addaleax commented Feb 6, 2023

nodejs-github-bot commented Feb 6, 2023

nodejs-github-bot commented Feb 6, 2023

nodejs-github-bot commented Feb 6, 2023

danielleadams commented Apr 2, 2023

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

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

addaleax Feb 6, 2023 •

edited

Loading