process: delay throwing an error using throwDeprecation

[BridgeAR]

This makes sure all warnings that were triggered before a deprecation
warning during the same tick are properly handled and logged.

It also guarantees that users can not catch the error anymore.

Fixes: #17871

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

[addaleax]

This is a pretty big breaking change to this feature, imo. I’m not sure that we should actually do this.

[BridgeAR]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2312/ ✅ (seems ok, it would be good to have someone else to take another look)

@addaleax what scenario do you see that's critical? It should be very unlikely that someone wraps the emitWarning call in a try / catch, don't you agree? I tried to find such cases on github and with gzemnid and could not find any code that did this (besides copies of Node.js internal code which wraps the unhandled rejections warning for unknown reasons. Potentially accessing the promise stack throwing? See #17158).

So what would change for a user? They would get all warnings from the same tick. The error would still end up as uncaught exception if the emit call itself is not wrapped (which is unlikely), just one tick later. The process would continue to run for one more tick but that should not have any influence on the user, since it's in general unclear when a deprecation message is triggered and when not.

[BridgeAR]

@nodejs/process PTAL

[nylen]

@BridgeAR Thanks for the PR. This is a big usability improvement for basically all async code that needs to abort the process when an unexpected exception occurs, like command-line scripts written using async and await.

From #17871 (comment):

When an error occurs in my code, I expect to see the place in my code where it occurred.

Using the following short script, I can confirm this PR fixes the issue I originally reported:

process.throwDeprecation = true;

async function x() {
	throw new Error( 'aaaa' );
}

x().then( r => console.log( r ) );

Behavior in Node v13.11.0

$ node test.js 
internal/process/warning.js:132
      throw warning;
      ^

DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
    at emitDeprecationWarning (internal/process/promises.js:161:11)
    at processPromiseRejections (internal/process/promises.js:213:13)
    at processTicksAndRejections (internal/process/task_queues.js:98:32) {
  name: 'DeprecationWarning',
  code: 'DEP0018'
}

Note, the error message and the stack trace are not helpful in diagnosing the issue that actually occurred.

Behavior in Node v13.11.0 + this PR

$ ~/code/node/out/Release/node test.js
(node:12380) UnhandledPromiseRejectionWarning: Error: aaaa
    at x (.../code/node-async-test/test.js:4:8)
    at Object.<anonymous> (.../code/node-async-test/test.js:7:1)
    at Module._compile (internal/modules/cjs/loader.js:1147:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1167:10)
    at Module.load (internal/modules/cjs/loader.js:996:32)
    at Function.Module._load (internal/modules/cjs/loader.js:896:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
    at internal/main/run_main_module.js:17:47
(node:12380) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 2)
internal/process/warning.js:135
        throw warning;
        ^

DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
    at emitDeprecationWarning (internal/process/promises.js:161:11)
    at processPromiseRejections (internal/process/promises.js:213:13)
    at processTicksAndRejections (internal/process/task_queues.js:98:32) {
  name: 'DeprecationWarning',
  code: 'DEP0018'
}

A bit verbose but works correctly (Error: aaaa at x (.../code/node-async-test/test.js:4:8)).

Summary

Currently in order to get a reasonable stack trace from such code, I need to put a block like the following in all scripts.

process.on( 'unhandledRejection', err => {
    console.error( 'Unhandled promise rejection:', err );
    process.exit( 1 );
} );

Please do fix this issue, since it will allow us to replace this with process.throwDeprecation = true; as one would expect.

By the way, when is throwDeprecation planned to be turned on by default?

[benjamingr]

Sorry but I'm with @addaleax here, this is a really scary change. I'm not blocking so I don't stall this - but this change needs to be done very carefully and potentially it's a big breaking change. If this lands this needs to be a semver-major.

[nylen]

@addaleax @benjamingr for those of us not as familiar with the Node.js internals, what might this break?

Another alternative would be to introduce this behavior as process.throwDeprecationFixed = true (strawman name). Something like this wouldn't require a new semver major

[benjamingr]

@nylen you can look at the test change in this PR to see what timing change happens here. This means that user code relying on the error being emitted in a particular way breaks. It means code will be getting the error at a different time and will be getting different errors. It also means postmortem analysis tools get a different heap snapshot and more.

I'm not against this change since I think throwing in nextTick is probably better here, and this is only for a secondary code path (throwing on deprecations and not for example unhandled rejections with strict). So I don't want to blow the "seriousness" of this out of proportion.

Also note that the particular issue (logging the unhandled rejection when throwing on deprecations) can be fixed ad-hoc.

You can work around the stack trace issue with --unhandled-rejections=strict instead which has a correct stack trace.

[nylen]

You can work around the stack trace issue with --unhandled-rejections=strict instead which has a correct stack trace.

I didn't know about this relatively new flag. This is almost perfect, but it looks like there is not a corresponding process.unhandledRejections = 'strict' setting at the code level, which means it's necessary to change the way that Node.js processes are launched instead of just changing their code.

If such a runtime flag could be added then I think that would resolve this issue nicely, without the backwards compatibility concerns. This mode also has much nicer / more concise output:

$ echo 'async function x() { throw new Error( "aaaa" ) }; x()' | node --unhandled-rejections=strict
[stdin]:1
async function x() { throw new Error( "aaaa" ) }; x()
                           ^

Error: aaaa
    at x ([stdin]:1:28)
    at [stdin]:1:51
    at Script.runInThisContext (vm.js:131:20)
    at Object.runInThisContext (vm.js:297:38)
    at Object.<anonymous> ([stdin]-wrapper:10:26)
    at Module._compile (internal/modules/cjs/loader.js:1147:30)
    at evalScript (internal/process/execution.js:94:25)
    at internal/main/eval_stdin.js:29:5
    at Socket.<anonymous> (internal/process/execution.js:207:5)
    at Socket.emit (events.js:327:22)

[benjamingr]

LGTM as semver major

[BridgeAR]

@nodejs/tsc this needs some reviews. This is IMO a semver-major bug fix.

CITGM looked good but since the results are not accessible anymore:
https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2384/

[mcollina]

lgtm

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[BridgeAR]

Landed in 49745cd

[nylen]

Thanks for landing this. I've opened an issue for a further improvement: #33577