New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

child_process: allow Infinity as maxBuffer value #10769

Merged
merged 2 commits into from Jan 17, 2017

Conversation

Projects
None yet
7 participants
@cjihrig
Contributor

cjihrig commented Jan 12, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

child_process

Closes #10767

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Jan 12, 2017

Member

+1 for feature, but needs docs and tests across all child_process APIs that support maxBuffer. Doesn't have to test that the behaviour is unlimited, but should test that it doesn't abort or throw arg typerrors.

Member

sam-github commented Jan 12, 2017

+1 for feature, but needs docs and tests across all child_process APIs that support maxBuffer. Doesn't have to test that the behaviour is unlimited, but should test that it doesn't abort or throw arg typerrors.

Show outdated Hide outdated test/parallel/test-child-process-spawnsync-validation-errors.js
fail('maxBuffer', -1, err);
fail('maxBuffer', NaN, err);
fail('maxBuffer', Infinity, err);
fail('maxBuffer', true, err);
fail('maxBuffer', false, err);
fail('maxBuffer', __dirname, err);

This comment has been minimized.

@jasnell

jasnell Jan 12, 2017

Member

Perhaps include a fail test for -Infinity

@jasnell

jasnell Jan 12, 2017

Member

Perhaps include a fail test for -Infinity

CHECK(js_max_buffer->IsUint32());
max_buffer_ = js_max_buffer->Uint32Value();
CHECK(js_max_buffer->IsNumber());
max_buffer_ = js_max_buffer->NumberValue();

This comment has been minimized.

@cjihrig

cjihrig Jan 12, 2017

Contributor

Hm, I'm not sure that JavaScript Infinity will convert to C++ properly here.

@cjihrig

cjihrig Jan 12, 2017

Contributor

Hm, I'm not sure that JavaScript Infinity will convert to C++ properly here.

This comment has been minimized.

@cjihrig

cjihrig Jan 12, 2017

Contributor

OK, so JavaScript Infinity translates to std::numeric_limits<double>::infinity() in C++. So, I changed max_buffer_ to a double.

@cjihrig

cjihrig Jan 12, 2017

Contributor

OK, so JavaScript Infinity translates to std::numeric_limits<double>::infinity() in C++. So, I changed max_buffer_ to a double.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Jan 12, 2017

Contributor

For consistency, we should probably add the same conditionals to the async counterparts.

Contributor

mscdex commented Jan 12, 2017

For consistency, we should probably add the same conditionals to the async counterparts.

Show outdated Hide outdated lib/child_process.js
@@ -473,7 +473,7 @@ function spawnSync(/*file, args, options*/) {
// Validate maxBuffer, if present.
if (options.maxBuffer != null &&
!(Number.isInteger(options.maxBuffer) && options.maxBuffer >= 0)) {
!(typeof options.maxBuffer === 'number' && options.maxBuffer >= 0)) {
throw new TypeError('"maxBuffer" must be an unsigned integer');

This comment has been minimized.

@sindresorhus

sindresorhus Jan 13, 2017

Error message needs to be updated to reflect the new constraint (It now accepts any positive number, not just unsigned integer).

@sindresorhus

sindresorhus Jan 13, 2017

Error message needs to be updated to reflect the new constraint (It now accepts any positive number, not just unsigned integer).

This comment has been minimized.

@jasnell

jasnell Jan 13, 2017

Member

That's a good point. The integer check is still required otherwise we'll have nonsense things like 0.1 accepted as the maxBuffer. The check should likely be something like:

if ((Number.isInteger(options.maxBuffer) || options.maxBuffer === Infinity) &&
    options.maxBuffer >= 0) {
  ...
}

And the error message should certainly be updated but that would upgrade this to a semver-major change

@jasnell

jasnell Jan 13, 2017

Member

That's a good point. The integer check is still required otherwise we'll have nonsense things like 0.1 accepted as the maxBuffer. The check should likely be something like:

if ((Number.isInteger(options.maxBuffer) || options.maxBuffer === Infinity) &&
    options.maxBuffer >= 0) {
  ...
}

And the error message should certainly be updated but that would upgrade this to a semver-major change

This comment has been minimized.

@cjihrig

cjihrig Jan 13, 2017

Contributor

0.1 is technically fine as a maxBuffer value, even if it is a bit weird. We're just going to be doing comparisons against the value to see if we surpass it.

And the error message should certainly be updated but that would upgrade this to a semver-major change

This builds on an existing semver major change, so this is going to be semver major regardless.

@cjihrig

cjihrig Jan 13, 2017

Contributor

0.1 is technically fine as a maxBuffer value, even if it is a bit weird. We're just going to be doing comparisons against the value to see if we surpass it.

And the error message should certainly be updated but that would upgrade this to a semver-major change

This builds on an existing semver major change, so this is going to be semver major regardless.

Show outdated Hide outdated lib/child_process.js
@@ -473,7 +473,7 @@ function spawnSync(/*file, args, options*/) {
// Validate maxBuffer, if present.
if (options.maxBuffer != null &&
!(Number.isInteger(options.maxBuffer) && options.maxBuffer >= 0)) {
!(typeof options.maxBuffer === 'number' && options.maxBuffer >= 0)) {
throw new TypeError('"maxBuffer" must be an unsigned integer');

This comment has been minimized.

@jasnell

jasnell Jan 13, 2017

Member

That's a good point. The integer check is still required otherwise we'll have nonsense things like 0.1 accepted as the maxBuffer. The check should likely be something like:

if ((Number.isInteger(options.maxBuffer) || options.maxBuffer === Infinity) &&
    options.maxBuffer >= 0) {
  ...
}

And the error message should certainly be updated but that would upgrade this to a semver-major change

@jasnell

jasnell Jan 13, 2017

Member

That's a good point. The integer check is still required otherwise we'll have nonsense things like 0.1 accepted as the maxBuffer. The check should likely be something like:

if ((Number.isInteger(options.maxBuffer) || options.maxBuffer === Infinity) &&
    options.maxBuffer >= 0) {
  ...
}

And the error message should certainly be updated but that would upgrade this to a semver-major change

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Jan 13, 2017

Contributor

Instead of "non-negative", can we use "positive"?

Contributor

mscdex commented Jan 13, 2017

Instead of "non-negative", can we use "positive"?

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Jan 13, 2017

Contributor

But zero is an allowed, non-positive value.

Contributor

cjihrig commented Jan 13, 2017

But zero is an allowed, non-positive value.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Jan 13, 2017

Contributor

It depends on which zero you're talking about ;-)

Contributor

mscdex commented Jan 13, 2017

It depends on which zero you're talking about ;-)

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Jan 13, 2017

Contributor

Ha. Ok, I'll make it just "positive"

Contributor

cjihrig commented Jan 13, 2017

Ha. Ok, I'll make it just "positive"

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig
Contributor

cjihrig commented Jan 16, 2017

@cjihrig

This comment has been minimized.

Show comment
Hide comment
Contributor

cjihrig commented Jan 17, 2017

cjihrig added some commits Jan 12, 2017

child_process: allow Infinity as maxBuffer value
Fixes: #10767
PR-URL: #10769
Reviewed-By: James M Snell <jasnell@gmail.com>
test: refactor test-child-process-spawnsync-maxbuf
This commit refactors test-child-process-spawnsync-maxbuf.js,
and adds testing for the case where maxBuffer is Infinity.

PR-URL: #10769
Reviewed-By: James M Snell <jasnell@gmail.com>

@cjihrig cjihrig merged commit 5b30c4f into nodejs:master Jan 17, 2017

@cjihrig cjihrig deleted the cjihrig:10767 branch Jan 17, 2017

italoacasas added a commit to italoacasas/node that referenced this pull request Jan 18, 2017

child_process: allow Infinity as maxBuffer value
Fixes: nodejs#10767
PR-URL: nodejs#10769
Reviewed-By: James M Snell <jasnell@gmail.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Jan 18, 2017

test: refactor test-child-process-spawnsync-maxbuf
This commit refactors test-child-process-spawnsync-maxbuf.js,
and adds testing for the case where maxBuffer is Infinity.

PR-URL: nodejs#10769
Reviewed-By: James M Snell <jasnell@gmail.com>

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Apr 12, 2017

Member

@cjihrig why is this semver-major? Is it because you added validation for some args?

Member

sam-github commented Apr 12, 2017

@cjihrig why is this semver-major? Is it because you added validation for some args?

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Apr 12, 2017

Contributor

Yes.

Contributor

cjihrig commented Apr 12, 2017

Yes.

@mscdex mscdex referenced this pull request Apr 23, 2017

Closed

net: use actual Timeout instances on Sockets #11154

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment