Encoding of child_process.exec.stdout changed from String to Buffer in v6.2.1

[sometimeskind]

• Version: v6.2.1
• Platform: Darwin Kernel Version 15.5.0
• Subsystem: child_process

Somewhere between node v6.2.0 and v6.2.1 the encoding of the data passed to the 'on data' callback for the child_process.exec.stdout stream seems to have changed from String to Buffer.

Was this an intentional change, and what is the recommended way of setting the encoding for the exec stdout/stdio stream?

Thanks

Code example:

var exec = require('child_process').exec;
var keepAlive = setInterval(() => {}, 1000000000);
var e = exec('ls');
e.stdout.on('data', function(data) {
  console.log('data: ', typeof data, data instanceof Buffer);
  clearInterval(keepAlive);
});

with v6.2.0, output is data: string false
with v6.2.1, output is data: object true

[cjihrig]

Was this an intentional change

I doubt this was an intended change. Without having bisected, my guess would be that the change happened in #6764.

and what is the recommended way of setting the encoding for the exec stdout/stdio stream?

e.stdout.setEncoding('utf8');

I would recommend not using exec() for this though. exec() is meant to run a process to completion, and then call back with the complete output.

[Trott]

Did the bisect, and @cjihrig is correct that #6764 introduced this bug.

[Trott]

Opened a pull request to add a known-issue test for this: #7375

[Trott]

I think the most sensible fix might be to revert that change from 6.x.x and put it in as a breaking change for 7.0.0 and update the documentation accordingly.

[cjihrig]

I wonder if it is worth a second breaking change to v6 in order to do the revert. Working around this is straightforward, and the bug in question goes against the primary use case of exec().

[Trott]

Working around this is straightforward

To work around it while preserving the old behavior, the default case will need to convert the string to a Buffer on every data event. I would expect that to be a significant performance hit if there are lots of data events. (Although I haven't coded it up so I haven't run benchmarks to confirm this....)

Or is there a different workaround you envision?

[cjihrig]

There probably would be a performance hit. Not sure how significant. To reiterate, I don't think handling individual data events is the intended/common use case for exec(). If we're going to put the same change back in v7, then it seems like a lot of flip-flopping of behavior.

[Trott]

I'll try to carve out a little time to code it up and benchmark to see the performance implications.

As far as flip-flopping, since it's flip-flopping on undocumented behavior that is definitely not the intended usage of exec() and for which there are better APIs already available, it may be OK to flip-flop. It would look like this:

• Up until 6.2.0, variable type is string.
• Oh, noes! Regression introduced in 6.2.1 that changed it to an object!
• 6.2.x or 6.3.0 reverts that change. We're back to string.
• 7.0.0 re-introduces the change.

While not ideal, I think that's OK if it's the only performant way to fix the maxBuffer bug.

EDIT: Oh, wait, I suspect (now that I think about it) that you are suggesting just leaving things as they are and sorta kinda declaring this Not-A-Bug? That works for me too, but since there is at least one example of Real World Breakage due to this, I am more inclined towards revert-and-reintroduce-at-semver-major-time.

[Trott]

Well, this is surprising but I guess that's why we benchmark: Reverting the fix in child_process.js and implementing Trott@3223341 instead seems to have no significant performance cost.

Without that change:

$ ~/test/node-performant-but-maxbuffer-bug benchmark/child_process/child-process-exec-stdout.js 
child_process/child-process-exec-stdout.js len="64" dur="5": 40737.35563
child_process/child-process-exec-stdout.js len="256" dur="5": 41020.86284
child_process/child-process-exec-stdout.js len="1024" dur="5": 40853.20079
child_process/child-process-exec-stdout.js len="4096" dur="5": 40846.50732
child_process/child-process-exec-stdout.js len="32768" dur="5": 40829.78559
$

With that change:

$ ~/test/node-non-performant benchmark/child_process/child-process-exec-stdout.js 
child_process/child-process-exec-stdout.js len="64" dur="5": 40833.82411
child_process/child-process-exec-stdout.js len="256" dur="5": 40828.31446
child_process/child-process-exec-stdout.js len="1024" dur="5": 40785.61719
child_process/child-process-exec-stdout.js len="4096" dur="5": 40832.74592
child_process/child-process-exec-stdout.js len="32768" dur="5": 40774.97655
$

/cc @mscdex on the benchmark just in case I'm Doing Something Wrong

Benefits of this fix:

• Keeps the (undocumented but used/expected in the wild) type for the handler arguments
• Fixes the maxBuffer bug

Downside:

• There's probably still a maxBuffer bug if someone does child.stdout.setEncoding(). That is probably fixable, though, with a check for child.stdout._readableState.decoder and correct action taken if it is found. I'm not sure we care much about performance of the code if someone is doing something like that, but we will want to benchmark it to make sure it doesn't impact the typical case. I am unable to introduce a problem with child.stdout.setEncoding(). If anyone thinks it's possible, sample code welcome.

[Trott]

Fixed via #7391