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

fs: throw more synchronous errors in JS land #19041

Closed
wants to merge 7 commits into from

Conversation

joyeecheung
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

fs

After this PR, all synchronous errors in node_file.cc are migrated into JS land, and they all have error tests in test-fs-error-messages.js (the asynchronous errors are tested as well).

The only synchronous errors left in fs is being fixed in #17851

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. labels Feb 27, 2018
@joyeecheung
Copy link
Member Author

@joyeecheung joyeecheung added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 27, 2018
size_t len;
int64_t pos;
const int argc = args.Length();
CHECK_GE(argc, 4);
Copy link
Member

Choose a reason for hiding this comment

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

5

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 1, 2018
@joyeecheung
Copy link
Member Author

Landed in 7cadb57...f7e5b38 with @targos 's comment addressed. Thanks!

@joyeecheung joyeecheung closed this Mar 2, 2018
joyeecheung added a commit that referenced this pull request Mar 2, 2018
PR-URL: #19041
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Mar 2, 2018
PR-URL: #19041
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Mar 2, 2018
PR-URL: #19041
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Mar 2, 2018
PR-URL: #19041
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Mar 2, 2018
PR-URL: #19041
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Mar 2, 2018
PR-URL: #19041
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Mar 2, 2018
PR-URL: #19041
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@joaocgreis
Copy link
Member

This PR landed with a full red Windows CI and has broken master. Should we revert or is it something simple to fix?

@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 3, 2018

@joaocgreis The windows CI seems to be broken by this

  c:\workspace\node-test-binary-windows>TASKKILL /F /IM node.exe /T   || TRUE
  ERROR: The process "node.exe" not found.

Not sure how it is related to this PR...

@richardlau
Copy link
Member

@joyeecheung e.g., https://ci.nodejs.org/job/node-test-binary-windows/15333/COMPILED_BY=vs2017,RUNNER=win10,RUN_SUBSET=0/console

Building addons
C:\Windows\SYSTEM32\cmd.exe[3240]: src\node_file.cc:1286: Assertion `(argc) == (7)' failed.
 1: node::DecodeWrite
 2: node::DecodeWrite
 3: v8::internal::ParseInfo::character_stream
 4: v8::internal::wasm::NativeModuleDeserializer::DeserializeFullBuffer
 5: v8::internal::LocalEmbedderHeapTracer::RequiresImmediateWrapperProcessing
 6: v8::internal::LocalEmbedderHeapTracer::RequiresImmediateWrapperProcessing
 7: v8::internal::LocalEmbedderHeapTracer::RequiresImmediateWrapperProcessing
 8: 000003FE44E04201

@joyeecheung
Copy link
Member Author

@richardlau Ah, so the ERROR: thing can be ignored since they are no real errors...didn't look down below that, sorry.

I think I have found the culprit..fix coming up.

joyeecheung added a commit to joyeecheung/node that referenced this pull request Mar 3, 2018
The binding writeBuffer has been changed in
nodejs#19041 and it now requires
the last argument to be a context object. makeSyncWrite
was not updated accordingly, resulting assertions on Windows.
This patch fixes the usage of writeBuffer there.
@joyeecheung
Copy link
Member Author

Fix in #19103

joyeecheung added a commit that referenced this pull request Mar 3, 2018
The binding writeBuffer has been changed in
#19041 and it now requires
the last argument to be a context object. makeSyncWrite
was not updated accordingly, resulting assertions on Windows.
This patch fixes the usage of writeBuffer there.

Also fix errors.uvException() so error.message are no longer
enumerable, this fixes the deepStrictEqual assertion on the
error object in test-stdout-close-catch.

PR-URL: #19103
Refs: #19041
Reviewed-By: Anna Henningsen <anna@addaleax.net>
alexeykuzmin added a commit to electron/node that referenced this pull request Apr 10, 2018
alexeykuzmin added a commit to electron/node that referenced this pull request Apr 27, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#19041
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#19041
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#19041
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#19041
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#19041
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#19041
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#19041
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
The binding writeBuffer has been changed in
nodejs#19041 and it now requires
the last argument to be a context object. makeSyncWrite
was not updated accordingly, resulting assertions on Windows.
This patch fixes the usage of writeBuffer there.

Also fix errors.uvException() so error.message are no longer
enumerable, this fixes the deepStrictEqual assertion on the
error object in test-stdout-close-catch.

PR-URL: nodejs#19103
Refs: nodejs#19041
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants