events: remove domain handling from events to domain #17403

vdeturckheim · 2017-11-30T17:18:47Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included (N/A)
documentation is changed or added (N/A)
commit message follows commit guidelines

Affected core subsystem(s)

Events and Domain I removed the domain handling in events to have domain doing that.
Relates to #17143

TimothyGu

Have you benchmarked any potential performance change this could bring for use cases of EventEmitter with domain enabled?

TimothyGu · 2017-11-30T17:21:11Z

lib/events.js

@@ -141,20 +116,13 @@ EventEmitter.prototype.emit = function emit(type, ...args) {
      err.context = er;
      throw err;
    }
-    return false;


Why did this get removed?

It has moved here https://github.com/nodejs/node/pull/17403/files/426afc70832c47f18f9afd9f90f35c1e554c8d3a#diff-0b93f5d72b0d34f5b972b661935344c3R415

The only case when this code was reached was when a domain was present, other situation throw.

addaleax

Nice!

vdeturckheim · 2017-11-30T17:26:57Z

lib/events.js

-      }
-      domain.emit('error', er);
-    } else if (er instanceof Error) {
+    if (er instanceof Error) {
      throw er; // Unhandled 'error' event
    } else {


I should remove the else here

addaleax · 2017-11-30T17:27:00Z

Have you benchmarked any potential performance change this could bring for use cases of EventEmitter with domain enabled?

I would be okay with a slight performance regression for a long-deprecated module like domain anyway.

vdeturckheim · 2017-11-30T17:27:42Z

@TimothyGu I have no data regarding perf impact here.

benjamingr

I'm +1 on the cleanup and not too concerned about performance implications.

benjamingr · 2017-11-30T17:35:18Z

Re: benchmarks. I don't think we have any for event emitters + domains.

addaleax · 2017-11-30T17:36:14Z

CI: https://ci.nodejs.org/job/node-test-commit/14486/

AndreasMadsen · 2017-11-30T19:32:38Z

lib/domain.js

@@ -387,3 +382,51 @@ Domain.prototype.bind = function(cb) {

  return runBound;
 };
+
+// communicate with events module, but don't require that


We should change this comment. It serves no purpose now, other than backward compatibility.

AndreasMadsen

Excellent. Only one minor nit.

TimothyGu · 2017-11-30T22:31:47Z

@addaleax

I would be okay with a slight performance regression for a long-deprecated module like domain anyway.

Yet, for a long time (years) domain was the only thing available that could do the things it could do. I don't foresee any significant performance problem that could stem from this PR, but would like to be safe.

@benjamingr

Re: benchmarks. I don't think we have any for event emitters + domains.

No need to even use domains in a benchmark, measuring the difference between EventEmitter usage with versus without require()ing it suffices.

TimothyGu

Change itself LGTM.

TimothyGu · 2017-11-30T22:33:04Z

lib/domain.js

@@ -387,3 +382,50 @@ Domain.prototype.bind = function(cb) {

  return runBound;
 };
+
+// override methods on event to have
+// EventEmitters going allong with domains


Nit: typos

// Override EventEmitter methods to make it domain-aware.

apapirovski

If this is only meant for 9.x & on, then instead of ...args I would recommend using arguments. There's no risk in V8 6.2 and it's faster, since we're just forwarding the full argument list with no slicing.

addaleax · 2017-12-01T19:33:02Z

CI: https://ci.nodejs.org/job/node-test-commit/14516/

apapirovski

Making my requests more explicit. Would like these changes to get made before it lands but if not, will open a PR afterwards.

apapirovski · 2017-12-04T14:12:30Z

lib/domain.js

+};
+
+const eventEmit = EventEmitter.prototype.emit;
+EventEmitter.prototype.emit = function emit(...args) {


@vdeturckheim Would you mind changing this to emit() and then using arguments everywhere? I realize performance is not that big of a deal but this will actually make a substantial difference.

@apapirovski are you sure it's faster? (In recent V8)

Yeah, I'm certain because I checked it very recently. ...args is only faster when arguments requires slicing or manually copying in situations like emit(type, ...args).

@apapirovski very interesting can you share a benchmark showing that? I'm pretty sure ...args is optimized.

cc @bmeurer

It has been optimized but in practical usage it still seems to perform worse. I've run into this working on nextTick, events and timers.

> function fn(...args) { ... return Reflect.apply(Math.max, Math, args); ... } > > function fn2() { ... return Reflect.apply(Math.max, Math, arguments); ... } > console.time('test'); for (var i = 0; i < 1e8; i++) { fn(1, 2, 3, 4); } console.timeEnd('test'); test: 279.502ms > console.time('test'); for (var i = 0; i < 1e8; i++) { fn2(1, 2, 3, 4); } console.timeEnd('test'); test: 284.195ms

The basic benchmark shows it being optimized. As soon as I've tried using it in practice where there's more than one layer to the usage, arguments seems to win out.

btw @benjamingr I decided to finally split test all the possible variations. Here are my findings:

function fn(...args) { cb(...args); }

is the fastest for that particular purpose, faster than using arguments (about 3-5% faster). But then there's:

function fn(something, ...args) { cb(...args); }

seems to depend on specific usage of args within fn. For example, I've found that in setTimeout and setImmediate we incur about 20-25% performance penalty compared to using manual copying of arguments.

On the flipside, it performs as expected within EventEmitter.prototype.emit... and is, in fact, yet again faster than using arguments. There definitely seem to be some edge cases around this behaviour that I don't quite understand.

I don't know if this helps with your plan to bug V8 devs regarding this :)

It's possible this might be related to whether args is being stored somewhere or just passed through to another function, or whether args is accessed at all in the body?

apapirovski · 2017-12-04T14:13:16Z

lib/domain.js

+EventEmitter.prototype.emit = function emit(...args) {
+  const domain = this.domain;
+  if (domain === null || domain === undefined || this === process) {
+    return eventEmit.apply(this, args);


Could you use Reflect.apply here (and everywhere else) to avoid any potentially overridden apply methods?

Why would we use Reflect.apply here but not in the regular event emitter code?

There's a PR introducing it there too, it's just currently blocked on some other stuff.

@apapirovski sounds good - although a little defensive (if someone overrides Function.prototype.apply or apply on a function object - they other didn't mean to or they know exactly what they're doing - in both cases I'm not sure Node.js should guard against it.

We definitely should. The usage of .apply is an implementation detail and should not be affected by monkey-patching.

Doesn't Reflect.apply have the same "issue"? It is writable too.

Of course. But it's a step better because we're no longer relying on a method of a user provided function. At least Reflect.apply allows us consistent behaviour between all the different functions that are going to be called, instead of relying on the potentially unique apply of each specific callback.

Strictly speaking, using some sort of an internal way of invoking functions brings us closer to the spec too (e.g. for timers).

Also, to elaborate, this has become less of an issue lately but it was potentially even more problematic in the past where certain functions were called as callback(arguments[0]) and others callback.apply(undefined, arguments).

vdeturckheim · 2017-12-04T15:41:14Z

@apapirovski since @addaleax placed the "ready" flag, I'm not certain I should still do changes :/

apapirovski · 2017-12-04T15:44:20Z

@vdeturckheim Up to you. ready just means it can land but if you want to make changes, you always can. Otherwise I'll just open a PR after this lands.

vdeturckheim · 2017-12-04T15:53:30Z

@apapirovski let's do that in two different PR then, I don't have access to my laptop atm.

apapirovski · 2017-12-04T16:17:14Z

Landing now. (Just running local tests one more time to make sure we're all good.)

apapirovski · 2017-12-04T16:32:35Z

Landed in cf4448c

Thanks!!

PR-URL: #17403 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Andreas Madsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

apapirovski · 2017-12-04T16:34:06Z

@addaleax or @AndreasMadsen do we land these changes to domain on 6.x or 8.x? Not sure what the situation is given the deprecation. Want to make sure we get the right labels on this, if necessary. Thanks!

novemberborn · 2017-12-10T16:26:16Z

@addaleax I think this is a bug in Node.js actually. We emit error events with non-Error instances. There is also a domain active. Despite having an event handler, the emitted error is considered unhandled. I reckon it's this line in this PR that's causing problems.

See https://gist.github.com/novemberborn/2d6137cab8a57fdbafbccc5b33926585. I've tested with node-v10.0.0-nightly20171209e9d1e1265a-darwin-x64.

I would not be aware that they use the domain module

Neither am I, so either that comes from node-tap or there's a dependency that enters a domain.

Fix an issue where error is never emitted on the original EventEmitter in situations where a listener for error does exist. Refactor to eliminate unnecessary try/catch/finally block. PR-URL: #17588 Refs: #17403 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs#17403 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Andreas Madsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

Fix an issue where error is never emitted on the original EventEmitter in situations where a listener for error does exist. Refactor to eliminate unnecessary try/catch/finally block. PR-URL: nodejs#17588 Refs: nodejs#17403 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2018-02-20T21:00:33Z

the commit neccessary to land after this 04ae486 does not land cleanly.

Would someone be willing to manually backport?

edit: cf4448c does not land cleanly either

Backport-PR-URL: #18487 PR-URL: #17403 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Andreas Madsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

Fix an issue where error is never emitted on the original EventEmitter in situations where a listener for error does exist. Refactor to eliminate unnecessary try/catch/finally block. Backport-PR-URL: #18487 PR-URL: #17588 Refs: #17403 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2018-02-22T20:37:24Z

@watson managed to track down this commit as breaking, potentially we should have considered it Semver-Major

Here is the reproducible case they provided https://gist.github.com/watson/d67b60629026fdb7d081be7477c040e5

The below code will pass on 9.5.0 and fail on 9.6.0

var assert = require('assert')
var EventsModule = require('events')
var descriptor = Object.getOwnPropertyDescriptor(EventsModule, 'usingDomains')
assert(descriptor)

I'm going to open a revert against the 9.x branch and potentially release another version later today. Thoughts?

vdeturckheim · 2018-02-22T20:44:56Z

@MylesBorins LGTM. Should it wait until v10 or should we try to do that in a non breaking change way in v9?

watson · 2018-02-22T20:52:02Z

Thanks for the quick action @MylesBorins 😃 The test-case was written before I knew what was causing the problem. It's code lifted from [email protected] but could be simplified to just require('assert')(require('events').usingDomains) 😜

The line setting this was removed in a previous commit. This potentially breaks code in the wild using this property. Refs: nodejs#17403 (comment)

The line setting this was removed in a previous commit. This potentially breaks code in the wild using this property. Refs: #17403 (comment) PR-URL: #18944 Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

The line setting this was removed in a previous commit. This potentially breaks code in the wild using this property. Refs: nodejs#17403 (comment) PR-URL: nodejs#18944 Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

nodejs-github-bot added domain Issues and PRs related to the domain subsystem. events Issues and PRs related to the events subsystem / EventEmitter. labels Nov 30, 2017

TimothyGu reviewed Nov 30, 2017

View reviewed changes

addaleax approved these changes Nov 30, 2017

View reviewed changes

vdeturckheim commented Nov 30, 2017

View reviewed changes

benjamingr approved these changes Nov 30, 2017

View reviewed changes

AndreasMadsen mentioned this pull request Nov 30, 2017

domain migration to async_hooks #17143

Open

7 tasks

AndreasMadsen reviewed Nov 30, 2017

View reviewed changes

AndreasMadsen approved these changes Nov 30, 2017

View reviewed changes

TimothyGu approved these changes Nov 30, 2017

View reviewed changes

TimothyGu reviewed Nov 30, 2017

View reviewed changes

events: remove domain handling from events to domain

1876e96

apapirovski approved these changes Dec 1, 2017

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 1, 2017

apapirovski reviewed Dec 4, 2017

View reviewed changes

apapirovski closed this Dec 4, 2017

addaleax added the dont-land-on-v6.x label Dec 4, 2017

addaleax added the dont-land-on-v9.x label Dec 10, 2017

apapirovski mentioned this pull request Dec 10, 2017

domain: fix error emit handling #17588

Closed

3 tasks

apapirovski removed the dont-land-on-v9.x label Jan 31, 2018

MylesBorins added the backport-requested-v9.x label Feb 20, 2018

MylesBorins added backported-to-v9.x and removed backport-requested-v9.x labels Feb 20, 2018

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

MylesBorins mentioned this pull request Feb 22, 2018

Breaking change landed in v9.6.0 nodejs/Release#312

Closed

MylesBorins mentioned this pull request Feb 22, 2018

events: preset usingDomains to false #18944

Closed

MylesBorins added a commit to MylesBorins/node that referenced this pull request Feb 22, 2018

events: preset usingDomains to false

ddf746c

The line setting this was removed in a previous commit. This potentially breaks code in the wild using this property. Refs: nodejs#17403 (comment)

gibfahn added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 22, 2018

richardlau mentioned this pull request Mar 2, 2018

Adding assertion for the test in test-event-emitter-subclass #19082

Closed

4 tasks

events: remove domain handling from events to domain #17403

events: remove domain handling from events to domain #17403

Conversation

vdeturckheim commented Nov 30, 2017

Checklist

Affected core subsystem(s)

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Nov 30, 2017

vdeturckheim commented Nov 30, 2017

benjamingr left a comment

Choose a reason for hiding this comment

benjamingr commented Nov 30, 2017

addaleax commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasMadsen left a comment

Choose a reason for hiding this comment

TimothyGu commented Nov 30, 2017

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski left a comment

Choose a reason for hiding this comment

addaleax commented Dec 1, 2017

apapirovski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Dec 6, 2017 • edited Loading

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

apapirovski Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

apapirovski Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

vdeturckheim commented Dec 4, 2017

apapirovski commented Dec 4, 2017

vdeturckheim commented Dec 4, 2017

apapirovski commented Dec 4, 2017 • edited Loading

apapirovski commented Dec 4, 2017

apapirovski commented Dec 4, 2017

novemberborn commented Dec 10, 2017

MylesBorins commented Feb 20, 2018 • edited Loading

MylesBorins commented Feb 22, 2018

vdeturckheim commented Feb 22, 2018

watson commented Feb 22, 2018

benjamingr Dec 4, 2017 •

edited

Loading

benjamingr Dec 4, 2017 •

edited

Loading

apapirovski Dec 6, 2017 •

edited

Loading

apapirovski Dec 4, 2017 •

edited

Loading

apapirovski Dec 4, 2017 •

edited

Loading

apapirovski commented Dec 4, 2017 •

edited

Loading

MylesBorins commented Feb 20, 2018 •

edited

Loading