src: fix fs.write() externalized string handling by bnoordhuis · Pull Request #18216 · nodejs/node

bnoordhuis · 2018-01-17T22:44:44Z

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/

joyeecheung · 2018-01-17T23:33:50Z

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 · 2018-01-20T14:59:21Z

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

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

bnoordhuis · 2018-01-23T00:24:45Z

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 · 2018-01-23T00:33:59Z

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 · 2018-01-23T18:19:09Z

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.

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 · 2018-02-20T22:25:57Z

b2b9d11 needs to be manually backported to v9.x

MylesBorins · 2018-02-27T06:34:52Z

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

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>

MylesBorins · 2018-05-23T17:56:26Z

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 · 2018-09-06T16:30:41Z

Backport is in #22731

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#18146Fixes: nodejs#22728 PR-URL: nodejs#18216

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

* 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: #18146Fixes: #22728 Backport-PR-URL: #22731 PR-URL: #18216 Reviewed-By: James M Snell <jasnell@gmail.com>

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

bnoordhuis mentioned this pull request Jan 17, 2018
fs.write does not work on external two-byte strings #18146
Closed

jasnell approved these changes Jan 17, 2018
View reviewed changes

joyeecheung approved these changes Jan 17, 2018
View reviewed changes

bnoordhuis force-pushed the fix18146 branch from ab97555 to f4747e4Compare January 20, 2018 14:55

bnoordhuis force-pushed the fix18146 branch from f4747e4 to c3d7e75Compare January 23, 2018 00:22

cjihrig approved these changes Jan 23, 2018
View reviewed changes

bnoordhuis added 2 commits January 23, 2018 19:17

bnoordhuis force-pushed the fix18146 branch from c3d7e75 to b2b9d11Compare January 23, 2018 18:18

bnoordhuis closed this Jan 23, 2018

bnoordhuis deleted the fix18146 branch January 23, 2018 18:18

bnoordhuis merged commit b2b9d11 into nodejs:masterJan 23, 2018

evanlucas mentioned this pull request Jan 30, 2018
v9.5.0 release proposal #18464
Merged

MylesBorins added the backport-requested-v9.x label Feb 20, 2018

MylesBorins added lts-watch-v6.x labels Feb 27, 2018

MylesBorins added backport-requested-v8.x and removed backport-requested-v9.x labels May 23, 2018

addaleax mentioned this pull request Sep 6, 2018
Large strings decoded from latin1 and then encoded to utf8 has wrong size #22728
Closed

addaleax mentioned this pull request Sep 6, 2018
[v8.x] src: fix fs.write() externalized string handling #22731
Closed

addaleax added backported-to-v8.x and removed backport-requested-v8.x labels Sep 6, 2018

BethGriggs mentioned this pull request Oct 30, 2018
v8.13.0 proposal #23974
Merged

beevelop mentioned this pull request Nov 20, 2018
2018-11-20 Version 8.13.0 'Carbon' (LTS) @MylesBorins prepared by @BethGriggs beevelop/docker-android-nodejs#250
Closed

Navigation Menu

Uh oh!

Uh oh!

src: fix fs.write() externalized string handling - #18216

src: fix fs.write() externalized string handling#18216
bnoordhuis merged 2 commits into
nodejs:masterfrom
bnoordhuis:fix18146

bnoordhuis commented Jan 17, 2018•
edited
Loading

Uh oh!

joyeecheung commented Jan 17, 2018•
edited
Loading

Uh oh!

bnoordhuis commented Jan 20, 2018

Uh oh!

bnoordhuis commented Jan 23, 2018

Uh oh!

jasnell commented Jan 23, 2018

Uh oh!

bnoordhuis commented Jan 23, 2018•
edited
Loading

Uh oh!

MylesBorins commented Feb 20, 2018

Uh oh!

MylesBorins commented Feb 27, 2018

Uh oh!

MylesBorins commented May 23, 2018

Uh oh!

addaleax commented Sep 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Uh oh!

Conversation

bnoordhuis commented Jan 17, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Jan 17, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented Jan 20, 2018

Uh oh!

bnoordhuis commented Jan 23, 2018

Uh oh!

jasnell commented Jan 23, 2018

Uh oh!

bnoordhuis commented Jan 23, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MylesBorins commented Feb 20, 2018

Uh oh!

MylesBorins commented Feb 27, 2018

Uh oh!

MylesBorins commented May 23, 2018

Uh oh!

addaleax commented Sep 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bnoordhuis commented Jan 17, 2018•
edited
Loading

joyeecheung commented Jan 17, 2018•
edited
Loading

bnoordhuis commented Jan 23, 2018•
edited
Loading