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.