events: make sure console functions exist #4479

davidvgalbraith · 2015-12-30T08:23:33Z

If there's no global console cached, these calls will initialize it,
which in some cases can cause a circular dependency, making the console
received here an empty object. The program shouldn't crash in this case.

Fixes #4467

if there's no global console cached, these calls will initialize it, which in some cases can cause a circular dependency, making the console received here an empty object. The program shouldn't crash in this case. Fixes nodejs#4467

davidvgalbraith · 2015-12-30T08:41:44Z

A more general fix would be to always eagerly cache the global console object. I am not sure what the startup performance tradeoff equation is with that, so I went with this minimal fix for the issue at hand.

jasnell · 2015-12-30T20:13:04Z

@nodejs/ctc

mscdex · 2015-12-30T23:44:40Z

test/parallel/test-global-console-exists.js

+const e = new events.EventEmitter();
+e.on('hello', function() {});
+
+assert.ok(!e._events['hello'].hasOwnProperty('warned'));


Perhaps it would be better to use a public API to check, such as:

assert.ok(!e.listeners('hello')[0].hasOwnProperty('warned'));

The listeners() API leaves out the warned property: https://github.com/nodejs/node/blob/master/lib/events.js#L393.

While I agree that it's the most efficient approach, using an internal API in a test is a bit troublesome. The "correct" way to do this would likely be to direct the stderr/stdout output to a stream and apply a regex to ensure that the result is as the user should expect it to be. It's a bit of a workaround but it ensures that we're testing exactly what the user would see.

davidvgalbraith · 2016-01-01T20:08:54Z

I thought up a better way to do this: now I include a reference to console in the setter for EventEmitter.defaultMaxListeners, so it'll always be compiled after you change the property. The advantage of this over my old approach is the old way wouldn't log the warning it got where console was uninitialized, whereas this approach always has the console initialized by the time it gets to logging the warning. It also was a strange leap of logic to have util.js and events.js accounting for a potentially uninitialized console, so I like this better.

jasnell · 2016-01-04T16:31:14Z

Much better approach. I always get a bit concerned when replacing bare properties with getter/setter but in this instance I cannot imagine that there'd be any issue. The actual code change LGTM but I think the test needs a bit more work to avoid using the internal API.

jasnell · 2016-01-04T16:31:44Z

Marking as a semver-minor due to the switch to using a getter/setter

davidvgalbraith · 2016-01-04T18:58:58Z

I refactored the test as per your suggestion @jasnell.

jasnell · 2016-01-08T17:03:15Z

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

jasnell · 2016-01-08T17:03:24Z

LGTM if CI is green

silverwind · 2016-01-11T19:07:44Z

test/parallel/test-global-console-exists.js

+
+const assert = require('assert');
+const events = require('events');
+const leak_warning = /EventEmitter memory leak detected. 2 hello listeners/;


Was this dot meant to be matched as a dot? You should escape it in that case.

silverwind · 2016-01-11T19:11:03Z

LGTM besides the two nits above.

eslint-disable required-modules escape regex dot

davidvgalbraith · 2016-01-11T20:43:47Z

Good ideas @silverwind, I've incorporated them.

silverwind · 2016-01-11T21:29:52Z

LGTM pending CI: https://ci.nodejs.org/job/node-test-pull-request/1199/

thefourtheye · 2016-01-11T23:49:38Z

test/parallel/test-global-console-exists.js

+const old_default = events.defaultMaxListeners;
+events.defaultMaxListeners = 1;
+
+const e = new events.EventEmitter();


Redundant EventEmitter

davidvgalbraith · 2016-01-12T00:14:40Z

Thanks @thefourtheye, updated.

silverwind · 2016-01-12T00:21:14Z

CI likes it. Still LGTM.

thefourtheye · 2016-01-12T02:39:39Z

test/parallel/test-global-console-exists.js

+e.on('hello', function() {});
+e.on('hello', function() {});
+
+assert.equal(write_calls, 2);


Shouldn't we also assert if the console object is properly initialized by now?

I don't think there's any way we can do that without incidentally initializing it. Our prior logging would have failed if console didn't initialize properly.

How about checking if console.error function exists by the time control reaches here?

Such a reference to console.error will compile console if it isn't already compiled.

I suppose we could inspect require('native_module')._cache.console.

Scratch that -- Error: Cannot find module 'native_module'

Hmmm, my office time has started. I'll think about this when I find time today. Let's not block this PR because of this. Perhaps you might want to leave a TODO or something in the test for the timebeing.

thefourtheye · 2016-01-12T02:41:35Z

test/parallel/test-global-console-exists.js

+  else if (write_calls === 1)
+    assert.ok(data.match(/Trace/));
+  else
+    assert.ok(false, 'stderr.write should be called only twice');


This is why we have assert.equal(write_calls, 2); at the end, right?

Sure! I felt like I should have some sort of catch-all else statement here though.

jasnell · 2016-01-15T01:48:43Z

test/parallel/test-global-console-exists.js

+e.on('hello', function() {});
+e.on('hello', function() {});
+
+// TODO validate console


Why add the comment vs. adding the validation?

As per earlier discussion (#4479 (diff)) I don't think it's actually possible to validate the console here in any significant way, but @thefourtheye requested a comment about validating it.

oh :-) lol completely missed that one

jasnell · 2016-01-15T02:20:47Z

LGTM

If there's no global console cached, initialize it. Fixes: #4467 PR-URL: #4479 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2016-01-15T02:21:58Z

Landed in f9a59c1...

@davidvgalbraith .. I squashed the commits and reworked the commit message to make sure it fit within our style guidelines. I also went ahead and expanded that TODO comment in the test case to make sure it describes exactly why we're leaving it as a TODO

davidvgalbraith · 2016-01-15T02:32:48Z

Thanks, @jasnell! I'm glad we got this one in.

If there's no global console cached, initialize it. Fixes: #4467 PR-URL: #4479 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * events: make sure console functions exist (Dave) #4479 * fs: add autoClose option to fs.createWriteStream (Saquib) #3679 * http: improves expect header handling (Daniel Sellers) #4501 * node: allow preload modules with -i (Evan Lucas) #4696 * v8,src: expose statistics about heap spaces (`v8.getHeapSpaceStatistics()`) (Ben Ripkens) #4463 * Minor performance improvements: - lib: Use arrow functions instead of bind where possible (Minwoo Jung) #3622 - module: cache stat() results more aggressively (Ben Noordhuis) #4575 - querystring: improve parse() performance (Brian White) #4675 PR-URL: #4742

If there's no global console cached, initialize it. Fixes: nodejs#4467 PR-URL: nodejs#4479 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * events: make sure console functions exist (Dave) nodejs#4479 * fs: add autoClose option to fs.createWriteStream (Saquib) nodejs#3679 * http: improves expect header handling (Daniel Sellers) nodejs#4501 * node: allow preload modules with -i (Evan Lucas) nodejs#4696 * v8,src: expose statistics about heap spaces (`v8.getHeapSpaceStatistics()`) (Ben Ripkens) nodejs#4463 * Minor performance improvements: - lib: Use arrow functions instead of bind where possible (Minwoo Jung) nodejs#3622 - module: cache stat() results more aggressively (Ben Noordhuis) nodejs#4575 - querystring: improve parse() performance (Brian White) nodejs#4675 PR-URL: nodejs#4742

events: make sure console functions exist

362d832

if there's no global console cached, these calls will initialize it, which in some cases can cause a circular dependency, making the console received here an empty object. The program shouldn't crash in this case. Fixes nodejs#4467

davidvgalbraith mentioned this pull request Dec 30, 2015

console.error is not a function #4467

Closed

JungMinu added the events Issues and PRs related to the events subsystem / EventEmitter. label Dec 30, 2015

JungMinu added lib / src Issues and PRs related to general changes in the lib or src directory. util Issues and PRs related to the built-in util module. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 30, 2015

mscdex reviewed Dec 30, 2015
View reviewed changes

initialize console in defaultMaxListeners setter

9f8eddb

davidvgalbraith force-pushed the console-error-is-a-function branch from 3e80ddd to 9f8eddb Compare January 1, 2016 20:07

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 4, 2016

test: overwrite stderr.write instead of checking e._events

04a9276

silverwind reviewed Jan 11, 2016
View reviewed changes

global console exists spec cleanups

ff4065e

eslint-disable required-modules escape regex dot

thefourtheye reviewed Jan 11, 2016
View reviewed changes

Use the default export EventEmitter from events

e1a14e7

thefourtheye reviewed Jan 12, 2016
View reviewed changes

TODO validate console

7152739

jasnell reviewed Jan 15, 2016
View reviewed changes

jasnell pushed a commit that referenced this pull request Jan 15, 2016

events: make sure console functions exist

f9a59c1

If there's no global console cached, initialize it. Fixes: #4467 PR-URL: #4479 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell closed this Jan 15, 2016

evanlucas mentioned this pull request Jan 18, 2016

v5.5.0 Release Proposal (Stable) #4742

Merged

evanlucas pushed a commit that referenced this pull request Jan 18, 2016

events: make sure console functions exist

a8330f7

If there's no global console cached, initialize it. Fixes: #4467 PR-URL: #4479 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: James M Snell <jasnell@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

events: make sure console functions exist #4479

events: make sure console functions exist #4479

davidvgalbraith commented Dec 30, 2015

davidvgalbraith commented Dec 30, 2015

jasnell commented Dec 30, 2015

mscdex Dec 30, 2015

davidvgalbraith Dec 30, 2015

jasnell Jan 4, 2016

davidvgalbraith commented Jan 1, 2016

jasnell commented Jan 4, 2016

jasnell commented Jan 4, 2016

davidvgalbraith commented Jan 4, 2016

jasnell commented Jan 8, 2016

jasnell commented Jan 8, 2016

silverwind Jan 11, 2016

silverwind commented Jan 11, 2016

davidvgalbraith commented Jan 11, 2016

silverwind commented Jan 11, 2016

thefourtheye Jan 11, 2016

davidvgalbraith commented Jan 12, 2016

silverwind commented Jan 12, 2016

thefourtheye Jan 12, 2016

davidvgalbraith Jan 12, 2016

thefourtheye Jan 12, 2016

davidvgalbraith Jan 12, 2016

davidvgalbraith Jan 12, 2016

davidvgalbraith Jan 12, 2016

thefourtheye Jan 12, 2016

thefourtheye Jan 12, 2016

davidvgalbraith Jan 12, 2016

jasnell Jan 15, 2016

davidvgalbraith Jan 15, 2016

jasnell Jan 15, 2016

jasnell commented Jan 15, 2016

jasnell commented Jan 15, 2016

davidvgalbraith commented Jan 15, 2016

events: make sure console functions exist #4479

events: make sure console functions exist #4479

Conversation

davidvgalbraith commented Dec 30, 2015

davidvgalbraith commented Dec 30, 2015

jasnell commented Dec 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidvgalbraith commented Jan 1, 2016

jasnell commented Jan 4, 2016

jasnell commented Jan 4, 2016

davidvgalbraith commented Jan 4, 2016

jasnell commented Jan 8, 2016

jasnell commented Jan 8, 2016

Choose a reason for hiding this comment

silverwind commented Jan 11, 2016

davidvgalbraith commented Jan 11, 2016

silverwind commented Jan 11, 2016

Choose a reason for hiding this comment

davidvgalbraith commented Jan 12, 2016

silverwind commented Jan 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Jan 15, 2016

jasnell commented Jan 15, 2016

davidvgalbraith commented Jan 15, 2016