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: truncate output when maxBuffer is exceeded #24951

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@Fishrock123
Copy link
Member

Fishrock123 commented Dec 11, 2018

Preserves truncated output for child_process.exec() when maxBuffer is exceeded.

This is particularly useful for commands which have indistinguishable
error codes for what output they produce.


I am presently running into this and just vendoring exec with a similar change into some code because... well, I need it, and don't want to re-write all of exec by hand.

Done as an added option / semver-minor because I want it sooner. IMO we should make this the default behavior but I think there's a chance something relies on the old behavior.
Done as patch as per discussion.

The old behavior was also completely untested so... commit 1 fixes that.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 11, 2018

@Fishrock123 I would argue this is a kind of bug fix and we do not need the option for it. We could just run CITGM and see if anyone relies on this behavior but I actually doubt that a lot of code relies on the specific error code and that the output is empty in such a case.

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Dec 12, 2018

@nodejs/child_process, and also @nodejs/citgm I suppose. (Since there's no way I can decipher output even if I start a run...)

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 12, 2018

I am happy to look into the CITGM result if you refactor the code / open an alternative one which adds the functionality without option.

@Fishrock123 Fishrock123 force-pushed the Fishrock123:child-process-exec-truncateMaxBuffer branch from 978d141 to c059ef0 Dec 12, 2018

@Fishrock123 Fishrock123 force-pushed the Fishrock123:child-process-exec-truncateMaxBuffer branch from c059ef0 to dfd1f12 Dec 12, 2018

@Fishrock123 Fishrock123 changed the title child_process: add truncateMaxBuffer option to exec() child_process: truncate output when maxBuffer is exceeded Dec 12, 2018

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Dec 12, 2018

Show resolved Hide resolved test/parallel/test-child-process-exec-maxBuffer.js Outdated

@Fishrock123 Fishrock123 force-pushed the Fishrock123:child-process-exec-truncateMaxBuffer branch from dfd1f12 to 31e35bd Dec 12, 2018

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Dec 15, 2018

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 15, 2018

Fresh CI: https://ci.nodejs.org/job/node-test-pull-request/19566/

Windows test failures look relevant.

@Trott
Copy link
Member

Trott left a comment

Blocking on relevant CI failures on Windows. Feel free to dismiss once those are addressed. I'm just blocking to make sure no one accidentally lands this before it's fixed.

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Dec 19, 2018

🤨(wat.)

(Why did 4 of the same test case run in fanned?)

anyways:

assert.js:86
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

+ '"hell'
- 'hello'
    at runChecks (c:\workspace\node-test-binary-windows\test\parallel\test-child-process-exec-maxBuffer.js:10:10)
    at cp.exec.common.mustCall (c:\workspace\node-test-binary-windows\test\parallel\test-child-process-exec-maxBuffer.js:31:7)
    at c:\workspace\node-test-binary-windows\test\common\index.js:335:15
    at ChildProcess.exithandler (child_process.js:301:5)
    at ChildProcess.emit (events.js:189:13)
    at maybeClose (internal/child_process.js:978:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:265:5)

So.... on windows (cmd.exe?) echo "x" returns "x" and not x?...

@Fishrock123 Fishrock123 force-pushed the Fishrock123:child-process-exec-truncateMaxBuffer branch from 31e35bd to 42db815 Dec 19, 2018

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Dec 19, 2018

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 19, 2018

@Trott Trott dismissed their stale review Dec 19, 2018

test no longer fails

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 31, 2018

No one has reviewed this since the last commit was pushed. @addaleax @BridgeAR Can you confirm that this still looks good to you? Anyone else want to review it? @nodejs/child_process

@Fishrock123 Are you still in Camp "I Like To Land My Own PRs, Please Don't Land Them For Me, KThxBai"? Or have you moved over to Camp "Sure, Whatevs, Land My PRs If You Want, Save Me The Trouble, Kewl"?

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Jan 4, 2019

I would prefer to land them myself to avoid surprises.

Anyways this should be ready to land? It's not clear to me if people are actually ok with this considering no one from the relevant domain has ever commented.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 4, 2019

Anyways this should be ready to land?

Technically, yes, I believe so.

It's not clear to me if people are actually ok with this considering no one from the relevant domain has ever commented.

If you'd be more comfortable getting the approval of someone specific, please do, obviously. (As you know, GitHub notifications can be overwhelming so it's sometimes a good idea to ping folks via other channels.)

@addaleax
Copy link
Member

addaleax left a comment

Yes, still LGTM

@BridgeAR
Copy link
Member

BridgeAR left a comment

Still LGTM

@Fishrock123 Fishrock123 force-pushed the Fishrock123:child-process-exec-truncateMaxBuffer branch from 42db815 to 3f6aebf Jan 12, 2019

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Jan 12, 2019

https://ci.nodejs.org/job/node-test-commit-arm/21379/nodes=ubuntu1604-arm64/testReport/(root)/test/known_issues_test_vm_timeout_escape_queuemicrotask/

test.known_issues/test-vm-timeout-escape-queuemicrotask
fail (0)
(node:54495) ExperimentalWarning: queueMicrotask() is experimental.
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 12, 2019

https://ci.nodejs.org/job/node-test-commit-arm/21379/nodes=ubuntu1604-arm64/testReport/(root)/test/known_issues_test_vm_timeout_escape_queuemicrotask/

test.known_issues/test-vm-timeout-escape-queuemicrotask
fail (0)
(node:54495) ExperimentalWarning: queueMicrotask() is experimental.

As always, some context would be helpful.

  • Are you asking a question about this failure? Or is this comment a statement and not a question?

  • Are you re-running the test on this platform?

  • Are you suggesting that it is related? Isn't related and is flaky? Neither being suggested?

  • Since it's your PR, what are you hoping others will do in response to this comment? Look at the test? Nothing and it's there just for your own note-taking? Something else?

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Jan 12, 2019

I am only noting that it happened. It is clearly unrelated.

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Jan 14, 2019

I don't know how to fix the CI here.... @Trott? Did I re-run this correctly? Do I still need to wait for something?

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 14, 2019

Selecting "Resume Build" from the left-hand nav on the node-test-commit job is usually the way to go.

Resume Build: https://ci.nodejs.org/job/node-test-commit/25046/

I have a PR in to fix the flaky test. #25503 Feel free to upvote the request for fast-tracking.

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Jan 15, 2019

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 17, 2019

Now that the pesky queue microtask test is fixed, let's re-run CI. Not using "Resume Build" because that doesn't rebase against current master, so it won't get the fix.

CI: https://ci.nodejs.org/job/node-test-pull-request/20175/

Fishrock123 added some commits Dec 11, 2018

test: add stdio checks to cp-exec-maxBuffer
Expands this test case to check what happens to stdout/stderr when
maxBuffer is exceeded.

Also changes how cases are checked so that assertion stacks are
tracable to their test case, aka 'make it actually debuggable'.
child_process: truncate output when maxBuffer is exceeded
Preserves truncated output for `child_process.exec()` when `maxBuffer`
is exceeded.

This is particularly useful for commands which have indistinguishable
error codes for what output they produce.

@Fishrock123 Fishrock123 force-pushed the Fishrock123:child-process-exec-truncateMaxBuffer branch from 3f6aebf to 2153075 Jan 23, 2019

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Jan 23, 2019

Landed in b1a4e41...e47f972

@Fishrock123 Fishrock123 deleted the Fishrock123:child-process-exec-truncateMaxBuffer branch Jan 23, 2019

Fishrock123 added a commit that referenced this pull request Jan 23, 2019

test: add stdio checks to cp-exec-maxBuffer
Expands this test case to check what happens to stdout/stderr when
maxBuffer is exceeded.

Also changes how cases are checked so that assertion stacks are
tracable to their test case, aka 'make it actually debuggable'.

PR-URL: #24951
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

Fishrock123 added a commit that referenced this pull request Jan 23, 2019

child_process: truncate output when maxBuffer is exceeded
Preserves truncated output for `child_process.exec()` when `maxBuffer`
is exceeded.

This is particularly useful for commands which have indistinguishable
error codes for what output they produce.

PR-URL: #24951
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

Trott added a commit to Trott/io.js that referenced this pull request Jan 24, 2019

test: fix pummel/test-exec
Fix test/pummel/test-exec.js which broke as a result of
e47f972
(nodejs#24951).

(Until very recently, pummel tests were not run at all in CI and
currently only run nightly on master.)

@Trott Trott referenced this pull request Jan 24, 2019

Closed

test: fix pummel/test-exec #25677

2 of 2 tasks complete

targos added a commit that referenced this pull request Jan 24, 2019

test: add stdio checks to cp-exec-maxBuffer
Expands this test case to check what happens to stdout/stderr when
maxBuffer is exceeded.

Also changes how cases are checked so that assertion stacks are
tracable to their test case, aka 'make it actually debuggable'.

PR-URL: #24951
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

targos added a commit that referenced this pull request Jan 24, 2019

child_process: truncate output when maxBuffer is exceeded
Preserves truncated output for `child_process.exec()` when `maxBuffer`
is exceeded.

This is particularly useful for commands which have indistinguishable
error codes for what output they produce.

PR-URL: #24951
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

Trott added a commit to Trott/io.js that referenced this pull request Jan 24, 2019

test: fix pummel/test-exec
Fix test/pummel/test-exec.js which broke as a result of
e47f972
(nodejs#24951).

(Until very recently, pummel tests were not run at all in CI and
currently only run nightly on master.)

PR-URL: nodejs#25677
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

MylesBorins added a commit that referenced this pull request Jan 24, 2019

test: fix pummel/test-exec
Fix test/pummel/test-exec.js which broke as a result of
e47f972
(#24951).

(Until very recently, pummel tests were not run at all in CI and
currently only run nightly on master.)

PR-URL: #25677
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

MylesBorins added a commit that referenced this pull request Jan 24, 2019

2019-01-224, Version 11.8.0 (Current)
Notable Changes:

* events:
  * For unhandled `error` events with an argument that is not an
    `Error` object, the resulting exeption will have more information
    about the argument.
    #25621
* child_process:
  * When the `maxBuffer` option is passed, `stdout` and `stderr` will
    be truncated rather than unavailable in case of an error.
    #24951
* policy:
  * Experimental support for module integrity checks through a manifest
    file is implemented now.
    #23834
* n-api:
  * The `napi_threadsafe_function` feature is now stable.
    #25556
* report:
  * An experimental diagnostic API for capturing process state is
    available as `process.report` and through command line flags.
    #22712
* tls:
  * `tls.connect()` takes a `timeout` option analogous to the
    `net.connect()` one.
    #25517
* worker:
  * `process.umask()` is available as a read-only function inside Worker
    threads now.
    #25526
  * An `execArgv` option that supports a subset of Node.js command line
    options is supported now.
    #25467

PR-URL: Coming Soon

@MylesBorins MylesBorins referenced this pull request Jan 24, 2019

Merged

v11.8.0 proposal #25687

MylesBorins added a commit that referenced this pull request Jan 24, 2019

2019-01-224, Version 11.8.0 (Current)
Notable Changes:

* events:
  * For unhandled `error` events with an argument that is not an
    `Error` object, the resulting exeption will have more information
    about the argument.
    #25621
* child_process:
  * When the `maxBuffer` option is passed, `stdout` and `stderr` will
    be truncated rather than unavailable in case of an error.
    #24951
* policy:
  * Experimental support for module integrity checks through a manifest
    file is implemented now.
    #23834
* n-api:
  * The `napi_threadsafe_function` feature is now stable.
    #25556
* report:
  * An experimental diagnostic API for capturing process state is
    available as `process.report` and through command line flags.
    #22712
* tls:
  * `tls.connect()` takes a `timeout` option analogous to the
    `net.connect()` one.
    #25517
* worker:
  * `process.umask()` is available as a read-only function inside Worker
    threads now.
    #25526
  * An `execArgv` option that supports a subset of Node.js command line
    options is supported now.
    #25467

PR-URL: #25687

MylesBorins added a commit that referenced this pull request Jan 24, 2019

2019-01-24, Version 11.8.0 (Current)
Notable Changes:

* events:
  * For unhandled `error` events with an argument that is not an
    `Error` object, the resulting exeption will have more information
    about the argument.
    #25621
* child_process:
  * When the `maxBuffer` option is passed, `stdout` and `stderr` will
    be truncated rather than unavailable in case of an error.
    #24951
* policy:
  * Experimental support for module integrity checks through a manifest
    file is implemented now.
    #23834
* n-api:
  * The `napi_threadsafe_function` feature is now stable.
    #25556
* report:
  * An experimental diagnostic API for capturing process state is
    available as `process.report` and through command line flags.
    #22712
* tls:
  * `tls.connect()` takes a `timeout` option analogous to the
    `net.connect()` one.
    #25517
* worker:
  * `process.umask()` is available as a read-only function inside Worker
    threads now.
    #25526
  * An `execArgv` option that supports a subset of Node.js command line
    options is supported now.
    #25467

PR-URL: #25687

MylesBorins added a commit that referenced this pull request Jan 24, 2019

2019-01-24, Version 11.8.0 (Current)
Notable Changes:

* events:
  * For unhandled `error` events with an argument that is not an
    `Error` object, the resulting exeption will have more information
    about the argument.
    #25621
* child_process:
  * When the `maxBuffer` option is passed, `stdout` and `stderr` will
    be truncated rather than unavailable in case of an error.
    #24951
* policy:
  * Experimental support for module integrity checks through a manifest
    file is implemented now.
    #23834
* n-api:
  * The `napi_threadsafe_function` feature is now stable.
    #25556
* report:
  * An experimental diagnostic API for capturing process state is
    available as `process.report` and through command line flags.
    #22712
* tls:
  * `tls.connect()` takes a `timeout` option analogous to the
    `net.connect()` one.
    #25517
* worker:
  * `process.umask()` is available as a read-only function inside Worker
    threads now.
    #25526
  * An `execArgv` option that supports a subset of Node.js command line
    options is supported now.
    #25467

PR-URL: #25687

MylesBorins added a commit that referenced this pull request Jan 25, 2019

2019-01-24, Version 11.8.0 (Current)
Notable Changes:

* events:
  * For unhandled `error` events with an argument that is not an
    `Error` object, the resulting exeption will have more information
    about the argument.
    #25621
* child_process:
  * When the `maxBuffer` option is passed, `stdout` and `stderr` will
    be truncated rather than unavailable in case of an error.
    #24951
* policy:
  * Experimental support for module integrity checks through a manifest
    file is implemented now.
    #23834
* n-api:
  * The `napi_threadsafe_function` feature is now stable.
    #25556
* report:
  * An experimental diagnostic API for capturing process state is
    available as `process.report` and through command line flags.
    #22712
* tls:
  * `tls.connect()` takes a `timeout` option analogous to the
    `net.connect()` one.
    #25517
* worker:
  * `process.umask()` is available as a read-only function inside Worker
    threads now.
    #25526
  * An `execArgv` option that supports a subset of Node.js command line
    options is supported now.
    #25467

PR-URL: #25687
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment