child_process: exec should return Buffers by default

[trevnorris]

Right now return values from child_process.exec() default to utf8. Instead the return value should be a buffer.

Side: Check maxBuffer in options to ensure it's not larger than Buffer will allow.

[tjfontaine]

Just to clarify the stdout and stderr arguments to the exit handler are always a string in v0.10 in v0.12 they should be a buffer

The return value will continue to be a child process object

[trevnorris]

Fixed by #5846

[balupton]

@trevnorris I believe this has regressed:

$ nvm use 0.12
Now using node v0.12.0
$ node -e "require('child_process').exec('node --version', function(err,stdout){console.log(stdout instanceof Buffer)})"
false

$ nvm use 0.10
Now using node v0.10.35
$ node -e "require('child_process').exec('node --version', function(err,stdout){console.log(stdout instanceof Buffer)})"
false

$ nvm use 0.8
Now using node v0.8.28
$ node -e "require('child_process').exec('node --version', function(err,stdout){console.log(stdout instanceof Buffer)})"
false

$ nvm use iojs
Now using io.js v1.5.1
$ node -e "require('child_process').exec('node --version', function(err,stdout){console.log(stdout instanceof Buffer)})"
false

Docs still indicate that stdout and stderr should be buffers, so is still an issue.

• 0.12 Docs Screenshot
• 0.10 Docs Screenshot

[balupton]

@trevnorris looking at the patch's code, it has landed:

$ nvm use 0.12
Now using node v0.12.0
$ node -e "require('child_process').exec('node --version', {encoding:null}, function(err,stdout){console.log(stdout instanceof Buffer)})"
true

$ nvm use 0.10
Now using node v0.10.35
16:41:08:[email protected]:/Users/balupton/Projects/safeps:master
$ node -e "require('child_process').exec('node --version', {encoding:null}, function(err,stdout){console.log(stdout instanceof Buffer)})"
false

However, it still isn't correct with the docs. The docs say that they should always be Buffers, not just Buffers when encoding is null.

Either the code needs to change, or the docs need to change.

[trevnorris]

@joyent/node-coreteam Thoughts on this. Do we update the code or the docs?

@balupton Thanks for bringing this to attention.

[piscisaureus]

From my perspective it makes sense to return buffers then encoding is null.
The question is whether this API change was warranted.
I would say that since it's already in node 0.12, we just want to update the docs.

[misterdjules]

@trevnorris My opinion is that, unless not returning buffers when no encoding is specified is a bug that breaks users, it should be a documentation change in v0.12. However, if we think that it's ultimately what we want, we should also submit a PR against master that makes exec pass buffers to the callback when no encoding is specified, or when a null encoding is passed.

[trevnorris]

@piscisaureus When == null or === null? Because undefined would then imply == null.

@misterdjules Honestly not sure how I take this. We have no test for it, and it's been broken for some time, but doesn't seem to be affecting many, if any, users. So doc change in v0.12?

[misterdjules]

@trevnorris Yes, let's do a doc change for v0.12. And if we still think that not specifying an encoding should eventually pass buffers to the callback, then let's do that in master, but we'll need an issue to track that unless we do it right away.

[trevnorris]

@misterdjules +1

[misterdjules]

@balupton @trevnorris @piscisaureus #9434.