Skip to content
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

cli: add --trace-exit cli option #30516

Closed
wants to merge 3 commits into from
Closed

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Nov 17, 2019

It could be convenient to trace abnormal exit of the Node.js processes
that printing stacktrace on each process.exit call with a cli option.
This also takes effects on worker threads.

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 added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 17, 2019
@legendecas legendecas added the cli Issues and PRs related to the Node.js command line interface. label Nov 17, 2019
doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Show resolved Hide resolved
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

src/env.cc Outdated Show resolved Hide resolved
src/env.cc Show resolved Hide resolved
@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 18, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

}

fprintf(
stderr, "WARNING: Exited the environment with code %d\n", exit_code);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we mark this as a warning and pipe to stderr?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other traces were printed as warnings and piped to stderr. Do you have any suggestions on the format of the traces?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to resolve this by yourself, I'm okay if this is a default behavior :)

fprintf(stderr, "(node:%d) ", uv_os_getpid());
} else {
fprintf(stderr, "(node:%d, thread:%llu) ", uv_os_getpid(), thread_id());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto, are this function called always from exceptions?

src/env.cc Show resolved Hide resolved
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Nov 23, 2019

It seems doc/node.1 also needs updating.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once the doc/node.1 is also updated

It could be convenient to trace abnormal exit of the Node.js processes
that printing stacktrace on each `process.exit` call with a cli option.
This also takes effects on worker threads.
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

doc/api/cli.md Outdated
added: REPLACEME
-->

Prints a stack trace whenever an environment is been exited proactively, i.e.
Copy link
Contributor

@Alhadis Alhadis Dec 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: is been should be has been (or simply remove the word been).

doc/node.1 Outdated
@@ -349,6 +349,10 @@ and
.It Fl -trace-events-enabled
Enable the collection of trace event tracing information.
.
.It Fl -trace-exit
Prints a stack trace whenever an environment is been exited proactively, i.e.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above; the typo has been made here too.

In addition, I recommend putting \& after i.e. to stop troff mistaking it as the end of a sentence. Doing so will stop an uncomfortable gap being inserted due to paragraph justification:

exited proactively, i.e.  invoking  # Without \&
exited proactively, i.e. invoking   # With \&

\& is Roff's “do nothing” character, which is helpful to suppress the behaviour of constructs that have syntactic meaning (for example, starting a line with a dot, which is Roff's way of identifying formatting instructions).

@addaleax
Copy link
Member

@legendecas Let me know if there’s anything I can do to help move this along, I’d definitely like to see it happen :)

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@legendecas legendecas added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 16, 2019
@addaleax
Copy link
Member

addaleax commented Dec 16, 2019

Landed in 6859fcf :)

@addaleax addaleax closed this Dec 16, 2019
addaleax pushed a commit that referenced this pull request Dec 16, 2019
It could be convenient to trace abnormal exit of the Node.js processes
that printing stacktrace on each `process.exit` call with a cli option.
This also takes effects on worker threads.

PR-URL: #30516
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@legendecas legendecas deleted the trace-exit branch December 17, 2019 02:10
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
It could be convenient to trace abnormal exit of the Node.js processes
that printing stacktrace on each `process.exit` call with a cli option.
This also takes effects on worker threads.

PR-URL: #30516
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 17, 2019
MylesBorins added a commit that referenced this pull request Dec 18, 2019
Notable Changes:

* cli:
  * add --trace-exit cli option (legendecas)
    #30516
* http,https:
  * increase server headers timeout (Tim Costa)
    #30071
* readline:
  * update ansi-regex (Ruben Bridgewater)
    #30907
  * promote \_getCursorPos to public api (Jeremy Albright)
    #30687
* repl:
  * add completion preview (Ruben Bridgewater)
    #30907
* util:
  * add Set and map size to inspect output (Ruben Bridgewater)
    #30225
* wasi:
  * require CLI flag to require() wasi module (Colin Ihrig)
    #30963

PR-URL: #31010
MylesBorins added a commit that referenced this pull request Dec 18, 2019
Notable Changes:

* cli:
  * add --trace-exit cli option (legendecas)
    #30516
* http,https:
  * increase server headers timeout (Tim Costa)
    #30071
* readline:
  * update ansi-regex (Ruben Bridgewater)
    #30907
  * promote \_getCursorPos to public api (Jeremy Albright)
    #30687
* repl:
  * add completion preview (Ruben Bridgewater)
    #30907
* util:
  * add Set and map size to inspect output (Ruben Bridgewater)
    #30225
* wasi:
  * require CLI flag to require() wasi module (Colin Ihrig)
    #30963

PR-URL: #31010
targos pushed a commit that referenced this pull request Jan 14, 2020
It could be convenient to trace abnormal exit of the Node.js processes
that printing stacktrace on each `process.exit` call with a cli option.
This also takes effects on worker threads.

PR-URL: #30516
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
It could be convenient to trace abnormal exit of the Node.js processes
that printing stacktrace on each `process.exit` call with a cli option.
This also takes effects on worker threads.

PR-URL: #30516
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
targos pushed a commit that referenced this pull request Feb 11, 2020
Notable changes:

New assert APIs

The `assert` module now provides experimental `assert.match()` and
`assert.doesNotMatch()` methods. They will validate that the first argument is a
string and matches (or does not match) the provided regular expression

This is an experimental feature.

Ruben Bridgewater [#30929](#30929).

Advanced serialization for IPC

The `child_process` and `cluster` modules now support a `serialization` option
to change the serialization mechanism used for IPC. The option can have one of
two values:

* `'json'` (default): `JSON.stringify()` and `JSON.parse()` are used. This is
  how message serialization was done before.
* `'advanced'`: The serialization API of the `v8` module is used. It is based on
  the HTML structured clone algorithm.
  and is able to serialize more built-in JavaScript object types, such as
  `BigInt`, `Map`, `Set` etc. as well as circular data structures.

Anna Henningsen [#30162](#30162).

CLI flags

The new `--trace-exit` CLI flag makes Node.js print a stack trace whenever the
Node.js environment is exited proactively (i.e. by invoking the `process.exit()`
function or pressing Ctrl+C).

legendecas [#30516](#30516).

___

The new `--trace-uncaught` CLI flag makes Node.js print a stack trace at the
time of throwing uncaught exceptions, rather than at the creation of the `Error`
object, if there is any.
This option is not enabled by default because it may affect garbage collection
behavior negatively.

Anna Henningsen [#30025](#30025).

___

The `--disallow-code-generation-from-strings` V8 CLI flag is now whitelisted in
the `NODE_OPTIONS` environment variable.

Shelley Vohr [#30094](#30094).

New crypto APIs

For DSA and ECDSA, a new signature encoding is now supported in addition to the
existing one (DER). The `verify` and `sign` methods accept a `dsaEncoding`
option, which can have one of two values:

* `'der'` (default): DER-encoded ASN.1 signature structure encoding `(r, s)`.
* `'ieee-p1363'`: Signature format `r || s` as proposed in IEEE-P1363.

Tobias Nießen [#29292](#29292).

___

A new method was added to `Hash`: `Hash.prototype.copy`. It makes it possible to
clone the internal state of a `Hash` object into a new `Hash` object, allowing
to compute the digest between updates.

Ben Noordhuis [#29910](#29910).

Dependency updates

libuv was updated to 1.34.0. This includes fixes to `uv_fs_copyfile()` and
`uv_interface_addresses()` and adds two new functions: `uv_sleep()` and
`uv_fs_mkstemp()`.

Colin Ihrig [#30783](#30783).

___

V8 was updated to 7.8.279.23. This includes performance improvements to object
destructuring, RegExp match failures and WebAssembly startup time.
The official release notes are available at https://v8.dev/blog/v8-release-78.

Michaël Zasso [#30109](#30109).

New EventEmitter APIs

The new `EventEmitter.on` static method allows to async iterate over events.

Matteo Collina [#27994](#27994).

___

It is now possible to monitor `'error'` events on an `EventEmitter` without
consuming the emitted error by installing a listener using the symbol
`EventEmitter.errorMonitor`.

Gerhard Stoebich [#30932](#30932).

___

Using `async` functions with event handlers is problematic, because it
can lead to an unhandled rejection in case of a thrown exception.

The experimental `captureRejections` option in the `EventEmitter` constructor or
the global setting change this behavior, installing a
`.then(undefined, handler)` handler on the `Promise`. This handler routes the
exception asynchronously to the `Symbol.for('nodejs.rejection')` method if there
is one, or to the `'error'` event handler if there is none.

Setting `EventEmitter.captureRejections = true` will change the default for all
new instances of `EventEmitter`.

This is an experimental feature.

Matteo Collina [#27867](#27867).

Performance Hooks are no longer experimental

The `perf_hooks` module is now considered a stable API.

legendecas [#31101](#31101).

Introduction of experimental WebAssembly System Interface (WASI) support

A new core module, `wasi`, is introduced to provide an implementation of the
[WebAssembly System Interface](https://wasi.dev/) specification.
WASI gives sandboxed WebAssembly applications access to the
underlying operating system via a collection of POSIX-like functions.

This is an experimental feature.

Colin Ihrig [#30258](#30258).

PR-URL: #31691
targos pushed a commit that referenced this pull request Feb 11, 2020
Notable changes:

New assert APIs

The `assert` module now provides experimental `assert.match()` and
`assert.doesNotMatch()` methods. They will validate that the first argument is a
string and matches (or does not match) the provided regular expression

This is an experimental feature.

Ruben Bridgewater [#30929](#30929).

Advanced serialization for IPC

The `child_process` and `cluster` modules now support a `serialization` option
to change the serialization mechanism used for IPC. The option can have one of
two values:

* `'json'` (default): `JSON.stringify()` and `JSON.parse()` are used. This is
  how message serialization was done before.
* `'advanced'`: The serialization API of the `v8` module is used. It is based on
  the HTML structured clone algorithm.
  and is able to serialize more built-in JavaScript object types, such as
  `BigInt`, `Map`, `Set` etc. as well as circular data structures.

Anna Henningsen [#30162](#30162).

CLI flags

The new `--trace-exit` CLI flag makes Node.js print a stack trace whenever the
Node.js environment is exited proactively (i.e. by invoking the `process.exit()`
function or pressing Ctrl+C).

legendecas [#30516](#30516).

___

The new `--trace-uncaught` CLI flag makes Node.js print a stack trace at the
time of throwing uncaught exceptions, rather than at the creation of the `Error`
object, if there is any.
This option is not enabled by default because it may affect garbage collection
behavior negatively.

Anna Henningsen [#30025](#30025).

___

The `--disallow-code-generation-from-strings` V8 CLI flag is now whitelisted in
the `NODE_OPTIONS` environment variable.

Shelley Vohr [#30094](#30094).

New crypto APIs

For DSA and ECDSA, a new signature encoding is now supported in addition to the
existing one (DER). The `verify` and `sign` methods accept a `dsaEncoding`
option, which can have one of two values:

* `'der'` (default): DER-encoded ASN.1 signature structure encoding `(r, s)`.
* `'ieee-p1363'`: Signature format `r || s` as proposed in IEEE-P1363.

Tobias Nießen [#29292](#29292).

___

A new method was added to `Hash`: `Hash.prototype.copy`. It makes it possible to
clone the internal state of a `Hash` object into a new `Hash` object, allowing
to compute the digest between updates.

Ben Noordhuis [#29910](#29910).

Dependency updates

libuv was updated to 1.34.0. This includes fixes to `uv_fs_copyfile()` and
`uv_interface_addresses()` and adds two new functions: `uv_sleep()` and
`uv_fs_mkstemp()`.

Colin Ihrig [#30783](#30783).

___

V8 was updated to 7.8.279.23. This includes performance improvements to object
destructuring, RegExp match failures and WebAssembly startup time.
The official release notes are available at https://v8.dev/blog/v8-release-78.

Michaël Zasso [#30109](#30109).

New EventEmitter APIs

The new `EventEmitter.on` static method allows to async iterate over events.

Matteo Collina [#27994](#27994).

___

It is now possible to monitor `'error'` events on an `EventEmitter` without
consuming the emitted error by installing a listener using the symbol
`EventEmitter.errorMonitor`.

Gerhard Stoebich [#30932](#30932).

___

Using `async` functions with event handlers is problematic, because it
can lead to an unhandled rejection in case of a thrown exception.

The experimental `captureRejections` option in the `EventEmitter` constructor or
the global setting change this behavior, installing a
`.then(undefined, handler)` handler on the `Promise`. This handler routes the
exception asynchronously to the `Symbol.for('nodejs.rejection')` method if there
is one, or to the `'error'` event handler if there is none.

Setting `EventEmitter.captureRejections = true` will change the default for all
new instances of `EventEmitter`.

This is an experimental feature.

Matteo Collina [#27867](#27867).

Performance Hooks are no longer experimental

The `perf_hooks` module is now considered a stable API.

legendecas [#31101](#31101).

Introduction of experimental WebAssembly System Interface (WASI) support

A new core module, `wasi`, is introduced to provide an implementation of the
[WebAssembly System Interface](https://wasi.dev/) specification.
WASI gives sandboxed WebAssembly applications access to the
underlying operating system via a collection of POSIX-like functions.

This is an experimental feature.

Colin Ihrig [#30258](#30258).

PR-URL: #31691
stderr.match(/WARNING: Exited the environment with code 0/g);
if (warnings === 0) {
assert.strictEqual(actualWarnings, null);
return;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This return; terminates the whole async function i.e. terminates the enclosing for cycle after the second iteration (i.e. the first iteration with warnings === 0). I assume that continue; should have been here instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for identifying this. Would you mind opening an issue for it (preferably with a snippet showing the bug if you have one)? It will get more attention as an issue that a comment here.

assert.strictEqual(actualWarnings.length, warnings);

if (variant.startsWith('worker')) {
const workerIds = stderr.match(/\(node:\d+, thread:\d+)/g);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This regular expression in invalid. It contains an unmatched ) i.e. ) at the end should be \). This was not noticed because there is a lazy syntax check of regular expressions in V8 and this statement is never reached because of the incorrect return; mentioned above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reporting this! I've submitted a fix for this #33769.

legendecas added a commit that referenced this pull request Jun 9, 2020
PR-URL: #33769
Fixes: #30516
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
codebytere pushed a commit that referenced this pull request Jun 18, 2020
PR-URL: #33769
Fixes: #30516
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
codebytere pushed a commit that referenced this pull request Jun 30, 2020
PR-URL: #33769
Fixes: #30516
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
codebytere pushed a commit that referenced this pull request Jul 10, 2020
PR-URL: #33769
Fixes: #30516
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
codebytere pushed a commit that referenced this pull request Jul 14, 2020
PR-URL: #33769
Fixes: #30516
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. cli Issues and PRs related to the Node.js command line interface. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.