★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/24951Nouvelle recherche | Portail wanayoo
Skip to content

/node

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 from
+75 −23

Conversation

Reviewers

@addaleax addaleax

@BridgeAR BridgeAR

@Trott Trott

Assignees
No one assigned
Labels
author ready child_process
Projects
None yet
Milestone
No milestone
5 participants
@Fishrock123 @nodejs-github-bot @BridgeAR @addaleax @Trott
@Fishrock123
Copy link
Member

commented Dec 11, 2018 •
edited

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
@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added the child_process label Dec 11, 2018

@Fishrock123 Fishrock123 added the semver-minor label Dec 11, 2018

@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member

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.

Sign in to view
Copy link
Member Author

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.

Sign in to view
Copy link
Member

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 removed the semver-minor label 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.

Sign in to view
Copy link
Member Author

commented Dec 12, 2018 •
edited by addaleax

@BridgeAR updated as per discussion

CI: https://ci.nodejs.org/job/node-test-pull-request/19457/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1682/
@BridgeAR

BridgeAR approved these changes Dec 12, 2018
View changes

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

addaleax approved these changes Dec 15, 2018
View changes

@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 15, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 15, 2018

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

Windows test failures look relevant.
@Trott

Trott reviewed Dec 15, 2018
View changes

Copy link
Member

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.

Sign in to view
Copy link
Member Author

commented Dec 19, 2018 •
edited

🤨(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.

Sign in to view
Copy link
Member Author

commented Dec 19, 2018

New CI: https://ci.nodejs.org/job/node-test-pull-request/19669/
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 19, 2018 •
edited

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19672/ ✔️

@Fishrock123 Fishrock123 added the author ready label Dec 19, 2018

@Trott Trott dismissed their stale review Dec 19, 2018

test no longer fails

@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

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.

Sign in to view
Copy link
Member Author

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.

Sign in to view
Copy link
Member

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

addaleax approved these changes Jan 4, 2019
View changes

Copy link
Member

left a comment

Yes, still LGTM
@BridgeAR

BridgeAR approved these changes Jan 10, 2019
View changes

Copy link
Member

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.

Sign in to view
Copy link
Member Author

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.

Sign in to view
Copy link
Member

commented Jan 12, 2019 •
edited

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.

Sign in to view
Copy link
Member Author

commented Jan 12, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Jan 14, 2019 •
edited

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.

Sign in to view
Copy link
Member

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.

Sign in to view
Copy link
Member

commented Jan 15, 2019

Resumed build: https://ci.nodejs.org/job/node-test-commit/25072/

This was referenced Jan 16, 2019

Closed
Open
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

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

@Fishrock123
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'.
e23d8f2
@Fishrock123
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.
Loading status checks…
2153075

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

@Fishrock123

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Jan 23, 2019

Landed in b1a4e41...e47f972

@Fishrock123 Fishrock123 closed this Jan 23, 2019

@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

@Fishrock123
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>
5f86682

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

@Fishrock123
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>
Loading status checks…
e47f972

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

@Trott
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.)
d78f29d

@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

@Fishrock123 @targos
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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
90a64ab

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

@Fishrock123 @targos
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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
ac5fa2c

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

@Trott
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>
0c6b5ce

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

@Trott @MylesBorins
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>
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
a9ffce9

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

@MylesBorins
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
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
Loading status checks…
e118153

@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

@MylesBorins
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
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
Loading status checks…
4422ab5

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

@MylesBorins
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
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
Loading status checks…
88e5ed8

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

@MylesBorins
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
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
Loading status checks…
11ee794

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

@MylesBorins
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
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
Loading status checks…
641de82
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.