fs: throw errors from sync branches instead of separate implementations by joyeecheung · Pull Request #49913 · nodejs/node

@nodejs-github-bot added c++

Issues and PRs that require attention from people who are familiar with C++.

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

Sep 28, 2023

@joyeecheung

Previously to throw errors from C++ land, sync versions of the fs
were created by copying C++ code from the original implementation
and moving JS code to a separate file. This can lead to several
problems:

1. By moving code to a new file for the sake of moving, it would
  be harder to use git blame to trace changes and harder to backport
  changes to older branches.
2. Scattering the async and sync versions of fs methods in
  different files makes it harder to keep them in sync and
  share code in the prologues and epilogues.
3. Having two copies of code doing almost the same thing results
  in duplication and can be prone to out-of-sync problems when the
  prologue and epilogue get updated.
4. There is a minor cost to startup in adding an additional file.
  This can add up even with the help of snapshots.

This patch moves the JS code back to lib/fs.js to stop 1, 2 & 4
and introduces C++ helpers SyncCallAndThrowIf() and
SyncCallAndThrowOnError() so that the original implementations
can be easily tweaked to allow throwing from C++ and stop 3.

anonrig

Qard

Qard approved these changes Sep 28, 2023

RaisinTen

benjamingr

tniessen

anonrig

anonrig

@joyeecheung

@anonrig anonrig added the commit-queue-squash

Add this label to instruct the Commit Queue to squash all the PR commits into the first one.

label

Sep 29, 2023

GeoffreyBooth pushed a commit to GeoffreyBooth/node that referenced this pull request

Oct 1, 2023
Previously to throw errors from C++ land, sync versions of the fs
were created by copying C++ code from the original implementation
and moving JS code to a separate file. This can lead to several
problems:

1. By moving code to a new file for the sake of moving, it would
  be harder to use git blame to trace changes and harder to backport
  changes to older branches.
2. Scattering the async and sync versions of fs methods in
  different files makes it harder to keep them in sync and
  share code in the prologues and epilogues.
3. Having two copies of code doing almost the same thing results
  in duplication and can be prone to out-of-sync problems when the
  prologue and epilogue get updated.
4. There is a minor cost to startup in adding an additional file.
  This can add up even with the help of snapshots.

This patch moves the JS code back to lib/fs.js to stop 1, 2 & 4
and introduces C++ helpers SyncCallAndThrowIf() and
SyncCallAndThrowOnError() so that the original implementations
can be easily tweaked to allow throwing from C++ and stop 3.

PR-URL: nodejs#49913
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

pluris pushed a commit to pluris/node that referenced this pull request

Oct 1, 2023

pluris pushed a commit to pluris/node that referenced this pull request

Oct 1, 2023

pluris pushed a commit to pluris/node that referenced this pull request

Oct 1, 2023

pluris pushed a commit to pluris/node that referenced this pull request

Oct 1, 2023

This was referenced

Oct 2, 2023

alexfernandez pushed a commit to alexfernandez/node that referenced this pull request

Nov 1, 2023
Previously to throw errors from C++ land, sync versions of the fs
were created by copying C++ code from the original implementation
and moving JS code to a separate file. This can lead to several
problems:

1. By moving code to a new file for the sake of moving, it would
  be harder to use git blame to trace changes and harder to backport
  changes to older branches.
2. Scattering the async and sync versions of fs methods in
  different files makes it harder to keep them in sync and
  share code in the prologues and epilogues.
3. Having two copies of code doing almost the same thing results
  in duplication and can be prone to out-of-sync problems when the
  prologue and epilogue get updated.
4. There is a minor cost to startup in adding an additional file.
  This can add up even with the help of snapshots.

This patch moves the JS code back to lib/fs.js to stop 1, 2 & 4
and introduces C++ helpers SyncCallAndThrowIf() and
SyncCallAndThrowOnError() so that the original implementations
can be easily tweaked to allow throwing from C++ and stop 3.

PR-URL: nodejs#49913
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

targos pushed a commit that referenced this pull request

Nov 11, 2023
Previously to throw errors from C++ land, sync versions of the fs
were created by copying C++ code from the original implementation
and moving JS code to a separate file. This can lead to several
problems:

1. By moving code to a new file for the sake of moving, it would
  be harder to use git blame to trace changes and harder to backport
  changes to older branches.
2. Scattering the async and sync versions of fs methods in
  different files makes it harder to keep them in sync and
  share code in the prologues and epilogues.
3. Having two copies of code doing almost the same thing results
  in duplication and can be prone to out-of-sync problems when the
  prologue and epilogue get updated.
4. There is a minor cost to startup in adding an additional file.
  This can add up even with the help of snapshots.

This patch moves the JS code back to lib/fs.js to stop 1, 2 & 4
and introduces C++ helpers SyncCallAndThrowIf() and
SyncCallAndThrowOnError() so that the original implementations
can be easily tweaked to allow throwing from C++ and stop 3.

PR-URL: #49913
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

codebytere added a commit to electron/electron that referenced this pull request

Dec 8, 2023

codebytere added a commit to electron/electron that referenced this pull request

Dec 8, 2023

codebytere added a commit to electron/electron that referenced this pull request

Dec 11, 2023
* chore: bump node in DEPS to v20.10.0

* chore: update feat_initialize_asar_support.patch

no code changes; patch just needed an update due to nearby upstream changes

Xref: nodejs/node#49986

* chore: update pass_all_globals_through_require.patch

no manual changes; patch applied with fuzz

Xref: nodejs/node#49657

* chore: update refactor_allow_embedder_overriding_of_internal_fs_calls

Xref: nodejs/node#49912

no code changes; patch just needed an update due to nearby upstream changes

* chore: update chore_allow_the_node_entrypoint_to_be_a_builtin_module.patch

Xref: nodejs/node#49986

minor manual changes needed to sync with upstream change

* update fix_expose_the_built-in_electron_module_via_the_esm_loader.patch

Xref: nodejs/node#50096
Xref: nodejs/node#50314
in lib/internal/modules/esm/load.js, update the code that checks for
`format === 'electron'`. I'd like 👀 on this

Xref: nodejs/node#49657
add braces in lib/internal/modules/esm/translators.js to sync with upstream

* fix: lazyload fs in esm loaders to apply asar patches

* nodejs/node#50127
* nodejs/node#50096

* esm: jsdoc for modules code

nodejs/node#49523

* test: set test-cli-node-options as flaky

nodejs/node#50296

* deps: update c-ares to 1.20.1

nodejs/node#50082

* esm: bypass CommonJS loader under --default-type=module

nodejs/node#49986

* deps: update uvwasi to 0.0.19

nodejs/node#49908

* lib,test: do not hardcode Buffer.kMaxLength

nodejs/node#49876

* crypto: account for disabled SharedArrayBuffer

nodejs/node#50034

* test: fix edge snapshot stack traces

nodejs/node#49659

* src: generate snapshot with --predictable

nodejs/node#48749

* chore: fixup patch indices

* fs: throw errors from sync branches instead of separate implementations

nodejs/node#49913

* crypto: ensure valid point on elliptic curve in SubtleCrypto.importKey

nodejs/node#50234

* esm: detect ESM syntax in ambiguous JavaScrip

nodejs/node#50096

* fixup! test: fix edge snapshot stack traces

* esm: unflag extensionless ES module JavaScript and Wasm in module scope

nodejs/node#49974

* [tagged-ptr] Arrowify objects

https://chromium-review.googlesource.com/c/v8/v8/+/4705331

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Charles Kerr <charles@charleskerr.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

debadree25 pushed a commit to debadree25/node that referenced this pull request

Apr 15, 2024
Previously to throw errors from C++ land, sync versions of the fs
were created by copying C++ code from the original implementation
and moving JS code to a separate file. This can lead to several
problems:

1. By moving code to a new file for the sake of moving, it would
  be harder to use git blame to trace changes and harder to backport
  changes to older branches.
2. Scattering the async and sync versions of fs methods in
  different files makes it harder to keep them in sync and
  share code in the prologues and epilogues.
3. Having two copies of code doing almost the same thing results
  in duplication and can be prone to out-of-sync problems when the
  prologue and epilogue get updated.
4. There is a minor cost to startup in adding an additional file.
  This can add up even with the help of snapshots.

This patch moves the JS code back to lib/fs.js to stop 1, 2 & 4
and introduces C++ helpers SyncCallAndThrowIf() and
SyncCallAndThrowOnError() so that the original implementations
can be easily tweaked to allow throwing from C++ and stop 3.

PR-URL: nodejs#49913
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>