child_process: make the maxBuffer correct with unicode #1902

JacksonTian · 2015-06-05T04:49:38Z

The maxLen just caculate the string length, but to unicode string,
the string size is less than buffer size.

cjihrig · 2015-06-05T15:05:06Z

Would it work (and be simpler) to just use Buffer.byteLength() here and here?

cjihrig · 2015-06-05T15:06:17Z

Ref: #1901

JacksonTian · 2015-06-05T16:57:17Z

caculate byte length maybe make performance slow. Would like to hear more suggestion.

trevnorris · 2015-06-05T16:59:10Z

Ditto on the byteLength() call. That's the (or at least should be) "standard" way.

JacksonTian · 2015-06-06T06:07:18Z

Thanks @cjihrig @trevnorris . Use byteLength now.

cjihrig · 2015-06-06T13:08:50Z

lib/child_process.js

@@ -230,7 +230,11 @@ exports.execFile = function(file /* args, options, callback */) {
  }

  child.stdout.addListener('data', function(chunk) {
-    stdoutLen += chunk.length;
+    if (!encoding) {


According to the docs, the unit of maxBuffer is bytes, so I think these should unconditionally use Buffer.byteLength().

@JacksonTian missed this one.

when don't set the encoding, the chunk is a buffer.

JacksonTian · 2015-06-06T15:54:57Z

@cjihrig updated.

JacksonTian · 2015-06-08T01:49:36Z

ping @nodejs/collaborators

cjihrig · 2015-06-08T01:55:38Z

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/778/

yosuke-furukawa · 2015-06-08T03:21:53Z

LGTM

cjihrig · 2015-06-08T03:23:27Z

CI is not happy.

yosuke-furukawa · 2015-06-08T03:31:21Z

oops. err not found in windows....
https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/778/nodes=win2008r2/tapTestReport/test.tap-23/

JacksonTian · 2015-06-08T03:44:01Z

the windows doesn't like unicode. hmmm.

JacksonTian · 2015-06-08T09:32:34Z

I think maybe reached an edge case for windows platform.

JacksonTian · 2015-06-08T13:10:15Z

Anyone can help me to restart the CI?

jbergstroem · 2015-06-08T23:29:27Z

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/781/

cjihrig · 2015-06-10T16:43:25Z

The test failures seem to be related to a known issue on Windows - nodejs/node-v0.x-archive#7940.

@piscisaureus is this something that can be skipped on Windows during testing?

JacksonTian · 2015-06-12T01:58:49Z

can we skip the test case for win32? ping @piscisaureus

brendanashworth · 2015-06-25T22:19:55Z

You can probably skip the test case with something like the crypto skip, but with common.isWindows.

JacksonTian · 2015-06-26T04:47:03Z

anyone can help me to restart the CI?

bnoordhuis · 2015-12-16T17:38:54Z

I think what @piscisaureus is getting at is that partial character sequences throw off the length calculation; the .setEncoding(encoding) call makes the stream convert bytes to characters and incomplete multi-byte sequences get buffered by the StringDecoder until more data is available (which may never come so it's imprecise at best.)

Apart from that stipulation the patch looks correct to me. It's rather inefficient though: we first convert from bytes to characters and then we convert that back to bytes for the length calculation.

JacksonTian · 2015-12-17T03:48:10Z

@bnoordhuis We can don't set encoding, so don't use StringDecoder, just only encode buffer before execute callback.

bnoordhuis · 2015-12-17T10:49:51Z

I think you will still need a StringDecoder instance to deal with partial character sequences. If you encode through just buf.toString(encoding), it's either going to throw an exception or lose bytes.

I don't think we can just remove the .setEncoding() calls; that would almost certainly break existing code. Perhaps it's possible to add a fast path to stream.Readable, presumably in readableAddChunk(), although that code is already a twisty little maze of if statements.

JacksonTian · 2016-01-15T08:53:45Z

With the commit bfb2cd0 be landed, the PR can be implemented more light-weight.

cjihrig · 2016-01-15T17:18:02Z

test/parallel/test-exec-max-buffer.js

+    maxBuffer: 10
+  }, common.mustCall(function(err, stdout, stderr) {
+    assert(err);
+    assert.equal(err.message, 'stdout maxBuffer exceeded');


Can you use assert.strictEqual() here. Also, you can drop the first assertion.

cjihrig · 2016-01-15T17:19:44Z

LGTM. This should hopefully avoid any encoding issues.

@bnoordhuis @piscisaureus @trevnorris thoughts?

The maxLen just caculate the string length, but to unicode string, the string size is less than buffer size.

Trott · 2016-03-10T22:58:44Z

Would it be possible to get another round of reviews from some combination of @bnoordhuis @piscisaureus and @trevnorris ? It would be nice to see this finally land if the objections have been sufficiently addressed. (And if not, it would be nice to have the objections documented or reiterated.)

Trott · 2016-03-10T22:59:29Z

Nit: Can you add Fixes: https://github.com/nodejs/node/issues/1901 metadata line to the commit message?

Trott · 2016-03-10T23:02:20Z

Nit: Can you find a way to shorten the first line of the commit message so that it is 50 characters or less?

trevnorris · 2016-03-11T00:15:50Z

For the test would it be possible to spawn a child node process and write to its stderr/stdout? That should work for all platforms.

bnoordhuis · 2016-03-23T16:57:46Z

It looks like the test still needs an update.

JacksonTian · 2016-03-24T02:58:31Z

bfb2cd0 was reverted, this issue needs hold.

jasnell · 2016-06-07T19:35:11Z

I believe this can be closed now given that #6764 landed.

JacksonTian · 2016-06-08T02:36:44Z

thanks @jasnell @Trott

mscdex added the child_process Issues and PRs related to the child_process subsystem. label Jun 5, 2015

JacksonTian force-pushed the max_buffer branch 2 times, most recently from d609ac1 to d90d98c Compare June 6, 2015 06:06

cjihrig reviewed Jun 6, 2015
View reviewed changes

JacksonTian force-pushed the max_buffer branch from d90d98c to a94dbc0 Compare June 6, 2015 15:22

JacksonTian force-pushed the max_buffer branch from a94dbc0 to 07682dd Compare June 8, 2015 13:04

rvagg force-pushed the master branch from 9d21135 to 628a3ab Compare June 25, 2015 09:18

JacksonTian force-pushed the max_buffer branch from 07682dd to 4aa24b8 Compare June 26, 2015 04:43

JacksonTian force-pushed the max_buffer branch 3 times, most recently from aa6a9d7 to 8e08e84 Compare December 17, 2015 06:35

JacksonTian force-pushed the max_buffer branch from 8e08e84 to 0bc6cdd Compare December 24, 2015 07:30

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:00

JacksonTian force-pushed the max_buffer branch from 0bc6cdd to 8d5a444 Compare January 15, 2016 08:42

JacksonTian force-pushed the max_buffer branch from 8d5a444 to 3e22109 Compare January 15, 2016 08:56

cjihrig reviewed Jan 15, 2016
View reviewed changes

child_process: make the maxBuffer correct with unicode

937452c

The maxLen just caculate the string length, but to unicode string, the string size is less than buffer size.

JacksonTian force-pushed the max_buffer branch from 3e22109 to 937452c Compare January 18, 2016 07:59

Trott mentioned this pull request Mar 10, 2016

the maxBuffer isn't complete accurate in child_process #1901

Closed

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:22

Trott added the stalled Issues and PRs that are stalled. label May 14, 2016

cjihrig mentioned this pull request May 16, 2016

child_process: measure buffer length in bytes #6764

Closed

3 tasks

jasnell closed this Jun 7, 2016

JacksonTian deleted the max_buffer branch May 4, 2017 08:07

child_process: make the maxBuffer correct with unicode #1902

child_process: make the maxBuffer correct with unicode #1902

Conversation

JacksonTian commented Jun 5, 2015

cjihrig commented Jun 5, 2015

cjihrig commented Jun 5, 2015

JacksonTian commented Jun 5, 2015

trevnorris commented Jun 5, 2015

JacksonTian commented Jun 6, 2015

cjihrig Jun 6, 2015

Choose a reason for hiding this comment

cjihrig Jun 6, 2015

Choose a reason for hiding this comment

JacksonTian Jun 6, 2015

Choose a reason for hiding this comment

JacksonTian commented Jun 6, 2015

JacksonTian commented Jun 8, 2015

cjihrig commented Jun 8, 2015

yosuke-furukawa commented Jun 8, 2015

cjihrig commented Jun 8, 2015

yosuke-furukawa commented Jun 8, 2015

JacksonTian commented Jun 8, 2015

JacksonTian commented Jun 8, 2015

JacksonTian commented Jun 8, 2015

jbergstroem commented Jun 8, 2015

cjihrig commented Jun 10, 2015

JacksonTian commented Jun 12, 2015

brendanashworth commented Jun 25, 2015

JacksonTian commented Jun 26, 2015

bnoordhuis commented Dec 16, 2015

JacksonTian commented Dec 17, 2015

bnoordhuis commented Dec 17, 2015

JacksonTian commented Jan 15, 2016

cjihrig Jan 15, 2016

Choose a reason for hiding this comment

cjihrig commented Jan 15, 2016

Trott commented Mar 10, 2016

Trott commented Mar 10, 2016

Trott commented Mar 10, 2016

trevnorris commented Mar 11, 2016

bnoordhuis commented Mar 23, 2016

JacksonTian commented Mar 24, 2016

jasnell commented Jun 7, 2016

JacksonTian commented Jun 8, 2016