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: fix fs.write() externalized string handling #18216

Merged
merged 2 commits into from
Jan 23, 2018

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Jan 17, 2018

  • Respect encoding argument when the string is externalized.

  • Copy the string when the write request can outlive the externalized
    string.

This commit removes StringBytes::GetExternalParts() because it is
fundamentally broken.

Fixes: #18146
CI: https://ci.nodejs.org/job/node-test-commit/15486/

@nodejs-github-bot 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. labels Jan 17, 2018
@joyeecheung
Copy link
Member

joyeecheung commented Jan 17, 2018

test-fs-write failed on power pc and s390x linux one

not ok 591 parallel/test-fs-write
  ---
  duration_ms: 0.88
  severity: fail
  stack: |-
    assert.js:68
      throw new errors.AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: '中文 1' strictEqual 'ⵎ蝥 ㄀'
        at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-fs-write.js:64:10)
        at Module._compile (module.js:660:30)
        at Object.Module._extensions..js (module.js:671:10)
        at Module.load (module.js:577:32)
        at tryModuleLoad (module.js:517:12)
        at Function.Module._load (module.js:509:3)
        at Function.Module.runMain (module.js:701:10)
        at startup (bootstrap_node.js:194:16)
        at bootstrap_node.js:640:3
  ...

Also doesn't compile on windows

src\string_bytes.cc(335): error C2589: '(': illegal token on right side of '::' [c:\workspace\node-compile-windows\node_lib.vcxproj]
src\string_bytes.cc(335): error C2062: type 'unknown-type' unexpected [c:\workspace\node-compile-windows\node_lib.vcxproj]
src\string_bytes.cc(335): error C2059: syntax error: ')' [c:\workspace\node-compile-windows\node_lib.vcxproj]
  agent.cc

@bnoordhuis
Copy link
Member Author

Added the missing include and skipped the shortcut on BE systems.

New CI: https://ci.nodejs.org/job/node-test-commit/15554

@bnoordhuis
Copy link
Member Author

Right, so there's a Windows build error because of a conflicting definition of max in <windows.h>. Added a commit that adds -DNOMINMAX to work around that.

For the curious: the reason std::min() works in other files is because node.h defines NOMINMAX but string_bytes.cc doesn't include that file.

@jasnell
Copy link
Member

jasnell commented Jan 23, 2018

because of a conflicting definition of max in <windows.h>

Yay! Windows!

On a serious note: thank you for the reminder on this. I knew this at one point but totally forgot about that discrepancy.

Build with `-DNOMINMAX` to stop `<windows.h>` from defining macros that
conflict with `std::min()` and `std::max()`.

PR-URL: nodejs#18216
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
* Respect `encoding` argument when the string is externalized.

* Copy the string when the write request can outlive the externalized
  string.

This commit removes `StringBytes::GetExternalParts()` because it is
fundamentally broken.

Fixes: nodejs#18146
PR-URL: nodejs#18216
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@bnoordhuis
Copy link
Member Author

bnoordhuis commented Jan 23, 2018

Landed in 9145388...b2b9d11. There was a bit of red in the CI but that's not unique to this PR, going by the last 24 hours.

evanlucas pushed a commit that referenced this pull request Jan 30, 2018
Build with `-DNOMINMAX` to stop `<windows.h>` from defining macros that
conflict with `std::min()` and `std::max()`.

PR-URL: #18216
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@MylesBorins
Copy link
Contributor

b2b9d11 needs to be manually backported to v9.x

@MylesBorins
Copy link
Contributor

I can confirm that v8.x and v6.x need this patch

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Build with `-DNOMINMAX` to stop `<windows.h>` from defining macros that
conflict with `std::min()` and `std::max()`.

PR-URL: nodejs#18216
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
* Respect `encoding` argument when the string is externalized.

* Copy the string when the write request can outlive the externalized
  string.

This commit removes `StringBytes::GetExternalParts()` because it is
fundamentally broken.

Fixes: nodejs#18146
PR-URL: nodejs#18216
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@MylesBorins
Copy link
Contributor

Can someone please backport this to v8.x-staging? If you are interested please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@addaleax
Copy link
Member

addaleax commented Sep 6, 2018

Backport is in #22731

addaleax pushed a commit to addaleax/node that referenced this pull request Oct 2, 2018
Build with `-DNOMINMAX` to stop `<windows.h>` from defining macros that
conflict with `std::min()` and `std::max()`.

PR-URL: nodejs#18216
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax pushed a commit to addaleax/node that referenced this pull request Oct 2, 2018
* Respect `encoding` argument when the string is externalized.

* Copy the string when the write request can outlive the externalized
  string.

This commit removes `StringBytes::GetExternalParts()` because it is
fundamentally broken.

Fixes: nodejs#18146
Fixes: nodejs#22728
PR-URL: nodejs#18216
BethGriggs pushed a commit that referenced this pull request Oct 16, 2018
Build with `-DNOMINMAX` to stop `<windows.h>` from defining macros that
conflict with `std::min()` and `std::max()`.

Backport-PR-URL: #22731
PR-URL: #18216
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

PR-URL: #22731
BethGriggs pushed a commit that referenced this pull request Oct 16, 2018
* Respect `encoding` argument when the string is externalized.

* Copy the string when the write request can outlive the externalized
  string.

This commit removes `StringBytes::GetExternalParts()` because it is
fundamentally broken.

Fixes: #18146
Fixes: #22728
Backport-PR-URL: #22731
PR-URL: #18216

Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Oct 30, 2018
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++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants