process: add 'warning' event #4782

jasnell · 2016-01-20T19:47:20Z

In several places throughout the code we write directly to stderr
to report warnings (deprecation, possible eventemitter memory leak).
The current design of simply dumping the text to stderr is less
than ideal. This PR introduces a new "Process Warnings" mechanism
that emits 'warn' events on the global process object. These are
invoked with a Warning object that is similar in structure to
an Error in that they have a name, message and stack trace.

By default, these warnings will be printed to stderr. This can be
suppressed using the --no-warnings command line flag, however the
'warn' event will still be emitted by the process, allowing applications
to handle the warnings in custom ways.

The --trace-warnings command line flag will tell Node.js to print
the full stack trace of warnings as part of the default handling.

The existing --no-deprecation, --throw-deprecation and
--trace-deprecation flags continue to work as they currently do,
but the exact output of the warning message is modified to occur
on process.nextTick(). The stack trace for the warning, however,
is preserved and shows the correct call site.

Test cases and documentation are included.

Refs: nodejs/node-eps#4 (comment)

cjihrig · 2016-01-20T20:14:47Z

I wonder if it might be simpler to just use an Error or extend Error instead of defining a new class. Obviously errors and warnings are different things, but I figured I'd throw the idea out there.

jasnell · 2016-01-20T20:15:57Z

considered that but wanted to make sure there was a clear differentiation between the two. I think it's important for users not to conflate warnings with errors.

Fishrock123 · 2016-01-20T21:46:39Z

Is there a reason we wouldn't just print warnings to stderr when stderr.isTTY?

jasnell · 2016-01-20T21:49:18Z

@Fishrock123 ... you mean as opposed to used console.error? Or something else?

Fishrock123 · 2016-01-20T21:49:57Z

No I mean why not just print by default when we're interacting with a console?

jasnell · 2016-01-20T21:52:55Z

That's what this does. In the few places through the code that we currently print warnings, we just do an immediate console.error or console.trace. This PR moves that into an event handler and provides a more generic framework to report other kinds of warnings... By default, tho, the default handler simply just dumps that out to console.error.

jasnell · 2016-01-20T21:58:09Z

unless I'm missing what you're saying :-).

Fishrock123 · 2016-01-21T00:09:06Z

@jasnell Right, ok, I see that now. So of course there will be concerns about this dumping warnings to log files, so what I was asking, or trying to suggest is something like https://github.com/rvagg/branch-diff/blob/v1.4.1/branch-diff.js#L77-L78 - i.e. only log when process.stderr.isTTY. Reason being you're unlikely to be doing actual logging things on a direct console, I think.

jasnell · 2016-01-21T04:33:01Z

Yeah I started looking at that after you asked. It's definitely a
possibility.
On Jan 20, 2016 4:09 PM, "Jeremiah Senkpiel" notifications@github.com
wrote:

@jasnell https://github.com/jasnell Right, ok, I see that now. So of
course there will be concerns about this dumping warnings to log files, so
what I was asking, or trying to suggest is something like
https://github.com/rvagg/branch-diff/blob/v1.4.1/branch-diff.js#L77-L78 -
i.e. only log when process.stderr.isTTY. Reason being you're unlikely to
be doing actual logging things on a direct console, I think.

—
Reply to this email directly or view it on GitHub
#4782 (comment).

benjamingr · 2016-01-21T15:36:48Z

doc/api/process.markdown

@@ -167,6 +167,55 @@ this, you can either attach a dummy `.catch(function() { })` handler to
 `resource.loaded`, preventing the `'unhandledRejection'` event from being
 emitted, or you can use the [`'rejectionHandled'`][] event.

+## Event: 'warn'
+
+Emitted whenever Node.js emits a "Process Warning". Process Warnings are


I think some examples of what process warnings here would be nice.

benjamingr · 2016-01-21T15:42:02Z

Very nice idea!

bnoordhuis · 2016-01-21T16:08:32Z

lib/internal/warn.js

+function emit(warning) {
+  process.nextTick(() => {
+    process.emit('warn', warning);
+  });


I'm not sure if emitting it asynchronously is actually a good idea; that arguably detaches it too much from the call site.

(On the other hand, synchronous events can result in unbounded recursion if the event handler itself generates warnings.)

This is one bit I'm not sure about either. The detaching from the callsite bit is exactly why the Warning object captures the stack trace when it is created.

The unbounded recursion could be stopped by not generating any more warning after a certain synchronous threshold.

Hmmm this seems fine but I don't know if I have the experience to validate that. Does anyone have specific problems that happen due to behavior like this and errors? (Well other than uncatchability, but we don't care about that here anyways.)

bnoordhuis · 2016-01-21T16:11:24Z

Left some comments. I don't know if it's really an improvement, I guess I'm -0.

Fishrock123 · 2016-01-21T16:56:06Z

I'm not really so sure what the value of exposing this as an API is?

I'd like to be able to have more warnings as I outlined above, but what would a module actually do with this other than print / log them?

jasnell · 2016-01-21T17:43:11Z

This tries to keep the new API surface to a minimum, just a new event and an error like object. The main thing this tries to accomplish is give a bit more flexibility in how those warnings are printed/logged. For instance, an application that uses a custom logging solution can register a handler to do custom logging of warnings so that they appear properly alongside other logged events:

const winston = require('winston');
// .. configure winston
process.on('warn', (warning) => {
  winston.warn(warning.message, warning);
});

Another example would be an application built on Electron that surfaces the warnings in the GUI somehow as opposed to the console (where they quite possibly wouldn't be seen).

benjamingr · 2016-01-21T19:01:27Z

I think it might be a good idea to make warnings a thing, document the hook and allow libraries to emit their own warnings in a standard way.

In second thought it's the sort of thing that needs to be discussed more in depth in an issue first.

jasnell · 2016-01-21T19:44:13Z

There's no particular rush on landing this so keeping it open for discussion for a while is just fine :-)

Fishrock123 · 2016-01-21T20:02:00Z

other example would be an application built on Electron that surfaces the warnings in the GUI somehow as opposed to the console (where they quite possibly wouldn't be seen).

Ah, that is an excellent note. cc @zcbenz What sort of API would be useful to you folks for warnings?

zcbenz · 2016-01-21T23:45:34Z

Ah, that is an excellent note. cc @zcbenz What sort of API would be useful to you folks for warnings?

The warn event looks perfect to me.

ronkorving · 2016-01-22T02:52:09Z

doc/api/process.markdown

+    > DeprecationWarning: (node:94742) util.debug is deprecated. Use
+    console.error instead.
+
+In contrast, the following example turns of the default warning output and


ronkorving · 2016-01-22T02:54:11Z

Great feature! I agree with @cjihrig though that an Error object would be more intuitive (despite the name). At least with an Error object I know which properties to expect (even without docs).

One question though, could it be made so that if there is a listener for "warn", it automatically doesn't print to stderr? Quite like how "error" and signal listening works. Listening turns off the default behavior.

jasnell · 2016-01-22T03:02:52Z

Yes, we can do that, however it may be a bit surprising. Users may not be
aware that a module has registered a listener and they could end up missing
warnings they need to see.

On Thu, Jan 21, 2016 at 6:54 PM, Ron Korving notifications@github.com
wrote:

Great feature! I agree with @cjihrig https://github.com/cjihrig though
that an Error object would be more intuitive (despite the name). At least
with an Error object I know which properties to expect (even without docs).

One question though, could it be made so that if there is a listener for
"warn", it automatically doesn't print to stderr? Quite like how error
and signal listening works. Listening turns off the default behavior.

—
Reply to this email directly or view it on GitHub
#4782 (comment).

jasnell · 2016-03-23T21:32:54Z

There were no objections raised on the CTC call. Giving this about 24 hours for any additional comments. Will land tomorrow afternoon.

In several places throughout the code we write directly to stderr to report warnings (deprecation, possible eventemitter memory leak). The current design of simply dumping the text to stderr is less than ideal. This PR introduces a new "process warnings" mechanism that emits 'warning' events on the global process object. These are invoked with a `warning` argument whose value is an Error object. By default, these warnings will be printed to stderr. This can be suppressed using the `--no-warnings` and `--no-deprecation` command line flags. For warnings, the 'warning' event will still be emitted by the process, allowing applications to handle the warnings in custom ways. The existing `--no-deprecation` flag will continue to supress all deprecation output generated by the core lib. The `--trace-warnings` command line flag will tell Node.js to print the full stack trace of warnings as part of the default handling. The existing `--no-deprecation`, `--throw-deprecation` and `--trace-deprecation` flags continue to work as they currently do, but the exact output of the warning message is modified to occur on process.nextTick(). The stack trace for the warnings and deprecations preserve and point to the correct call site. A new `process.emitWarning()` API is provided to permit userland to emit warnings and deprecations using the same consistent mechanism. Test cases and documentation are included.

jasnell · 2016-03-24T19:11:29Z

Commits squashed. One final CI run before landing: https://ci.nodejs.org/job/node-test-pull-request/2059/

In several places throughout the code we write directly to stderr to report warnings (deprecation, possible eventemitter memory leak). The current design of simply dumping the text to stderr is less than ideal. This PR introduces a new "process warnings" mechanism that emits 'warning' events on the global process object. These are invoked with a `warning` argument whose value is an Error object. By default, these warnings will be printed to stderr. This can be suppressed using the `--no-warnings` and `--no-deprecation` command line flags. For warnings, the 'warning' event will still be emitted by the process, allowing applications to handle the warnings in custom ways. The existing `--no-deprecation` flag will continue to supress all deprecation output generated by the core lib. The `--trace-warnings` command line flag will tell Node.js to print the full stack trace of warnings as part of the default handling. The existing `--no-deprecation`, `--throw-deprecation` and `--trace-deprecation` flags continue to work as they currently do, but the exact output of the warning message is modified to occur on process.nextTick(). The stack trace for the warnings and deprecations preserve and point to the correct call site. A new `process.emitWarning()` API is provided to permit userland to emit warnings and deprecations using the same consistent mechanism. Test cases and documentation are included. PR-URL: #4782 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Wyatt Preul <wpreul@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

jasnell · 2016-03-24T20:21:24Z

Landed in c6656db. Thank you all!

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * OS X * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 [#6402](#6402). * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

Summary: Add declaration and tests for [process.emitWarning](https://nodejs.org/api/process.html#process_process_emitwarning_warning_name_ctor) (introduced in v6.0.0 nodejs/node#4782) Closes #3248 Reviewed By: calebmer Differential Revision: D4861198 fbshipit-source-id: 926a288308301a09656d71f13c7346dff95f9338

boneskull · 2018-03-20T17:33:16Z

@jasnell Why is process.emit('warning') called on next tick (by default)?

jasnell · 2018-03-20T17:35:12Z

Just to keep the processing of the event from negatively impacting the primary sync code flow... just defers it a bit.

jasnell added semver-major PRs that contain breaking changes and should be released in the next major version. process Issues and PRs related to the process subsystem. labels Jan 20, 2016

benjamingr reviewed Jan 21, 2016
View reviewed changes

bnoordhuis reviewed Jan 21, 2016
View reviewed changes

ronkorving reviewed Jan 22, 2016
View reviewed changes

jasnell force-pushed the process-warning branch from b9130c3 to 66b1ea3 Compare March 24, 2016 19:10

jasnell removed the ctc-agenda label Mar 24, 2016

jasnell closed this Mar 24, 2016

jasnell mentioned this pull request Apr 19, 2016

What is new in v6? #6264

Closed

jasnell mentioned this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release #6383

Closed

davecardwell mentioned this pull request Jan 23, 2017

Add declaration for node.js’ process.emitWarning facebook/flow#3248

Closed

dougwilson mentioned this pull request Mar 3, 2017

Should util.deprecate() be removed from the public API? #11642

Closed

apapirovski mentioned this pull request May 14, 2018

process: refactor emitWarning #20726

Closed

4 tasks

dougwilson mentioned this pull request Nov 7, 2018

Respect --no-deprecation and process.noDeprecation dougwilson/nodejs-depd#29

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

process: add 'warning' event #4782

process: add 'warning' event #4782

jasnell commented Jan 20, 2016

cjihrig commented Jan 20, 2016

jasnell commented Jan 20, 2016

Fishrock123 commented Jan 20, 2016

jasnell commented Jan 20, 2016

Fishrock123 commented Jan 20, 2016

jasnell commented Jan 20, 2016

jasnell commented Jan 20, 2016

Fishrock123 commented Jan 21, 2016

jasnell commented Jan 21, 2016

benjamingr Jan 21, 2016

jasnell Jan 23, 2016

benjamingr commented Jan 21, 2016

bnoordhuis Jan 21, 2016

jasnell Jan 21, 2016

benjamingr Jan 21, 2016

Fishrock123 Jan 22, 2016

bnoordhuis commented Jan 21, 2016

Fishrock123 commented Jan 21, 2016

jasnell commented Jan 21, 2016

benjamingr commented Jan 21, 2016

jasnell commented Jan 21, 2016

Fishrock123 commented Jan 21, 2016

zcbenz commented Jan 21, 2016

ronkorving Jan 22, 2016

ronkorving commented Jan 22, 2016

jasnell commented Jan 22, 2016

jasnell commented Mar 23, 2016

jasnell commented Mar 24, 2016

jasnell commented Mar 24, 2016

boneskull commented Mar 20, 2018

jasnell commented Mar 20, 2018

process: add 'warning' event #4782

process: add 'warning' event #4782

Conversation

jasnell commented Jan 20, 2016

cjihrig commented Jan 20, 2016

jasnell commented Jan 20, 2016

Fishrock123 commented Jan 20, 2016

jasnell commented Jan 20, 2016

Fishrock123 commented Jan 20, 2016

jasnell commented Jan 20, 2016

jasnell commented Jan 20, 2016

Fishrock123 commented Jan 21, 2016

jasnell commented Jan 21, 2016

benjamingr Jan 21, 2016

Choose a reason for hiding this comment

jasnell Jan 23, 2016

Choose a reason for hiding this comment

benjamingr commented Jan 21, 2016

bnoordhuis Jan 21, 2016

Choose a reason for hiding this comment

jasnell Jan 21, 2016

Choose a reason for hiding this comment

benjamingr Jan 21, 2016

Choose a reason for hiding this comment

Fishrock123 Jan 22, 2016

Choose a reason for hiding this comment

bnoordhuis commented Jan 21, 2016

Fishrock123 commented Jan 21, 2016

jasnell commented Jan 21, 2016

benjamingr commented Jan 21, 2016

jasnell commented Jan 21, 2016

Fishrock123 commented Jan 21, 2016

zcbenz commented Jan 21, 2016

ronkorving Jan 22, 2016

Choose a reason for hiding this comment

ronkorving commented Jan 22, 2016

jasnell commented Jan 22, 2016

jasnell commented Mar 23, 2016

jasnell commented Mar 24, 2016

jasnell commented Mar 24, 2016

boneskull commented Mar 20, 2018

jasnell commented Mar 20, 2018