events: remove reaches into _events internals

Refactor lib & src code to eliminate all deep reaches into the internal _events dictionary object, instead use available APIs and add an extra argument unwrap to listeners.
nodejs · Nov 26, 2017 · 7ec336b · 7ec336b
1 parent 7aca9c8
commit 7ec336b
Show file tree

Hide file tree

Showing 11 changed files with 85 additions and 89 deletions.
diff --git a/doc/api/events.md b/doc/api/events.md
@@ -344,16 +344,23 @@ added: v3.2.0
 
 Returns the number of listeners listening to the event named `eventName`.
 
-### emitter.listeners(eventName)
+### emitter.listeners(eventName[, unwrap])
 <!-- YAML
 added: v0.1.26
 changes:
   - version: v7.0.0
     pr-url: https://github.com/nodejs/node/pull/6881
     description: For listeners attached using `.once()` this returns the
                  original listeners instead of wrapper functions now.
+  - version: REPLACEME
+    pr-url: REPLACEME
+    description: Second optional argument `unwrap` allows the wrapped
+                 listeners to be returned (such as ones created with
+                 `.once()`)
 -->
-- `eventName` {any}
+* `eventName` {any} The name of the event.
+* `unwrap` {boolean} When set to true, will return the original listeners
+  instead of the wrapper functions created by `.once()`. **Default:** `true`
 
 Returns a copy of the array of listeners for the event named `eventName`.
 

diff --git a/lib/domain.js b/lib/domain.js
@@ -94,6 +94,9 @@ class Domain extends EventEmitter {
 
     this.members = [];
     asyncHook.enable();
+
+    this.on('newListener', setHasErrorListener);
+    this.on('removeListener', removeHasErrorListener);
   }
 }
 
@@ -105,6 +108,19 @@ exports.create = exports.createDomain = function() {
 
 // the active domain is always the one that we're currently in.
 exports.active = null;
+
+function setHasErrorListener(type) {
+  if (type === 'error')
+    this._hasErrorListener = true;
+}
+
+function removeHasErrorListener(type) {
+  if (type === 'error' && !this.listenerCount('error'))
+    this._hasErrorListener = false;
+}
+
+Domain.prototype._hasErrorListener = false;
+
 Domain.prototype.members = undefined;
 
 // Called by process._fatalException in case an error was thrown.

diff --git a/lib/events.js b/lib/events.js
@@ -400,7 +400,7 @@ EventEmitter.prototype.removeAllListeners =
       return this;
     };
 
-EventEmitter.prototype.listeners = function listeners(type) {
+EventEmitter.prototype.listeners = function listeners(type, unwrap = true) {
   const events = this._events;
 
   if (events === undefined)
@@ -413,7 +413,7 @@ EventEmitter.prototype.listeners = function listeners(type) {
   if (typeof evlistener === 'function')
     return [evlistener.listener || evlistener];
 
-  return unwrapListeners(evlistener);
+  return unwrap ? unwrapListeners(evlistener) : arrayClone(evlistener);
 };
 
 EventEmitter.listenerCount = function(emitter, type) {
@@ -459,6 +459,13 @@ EventEmitter.prototype.eventNames = function eventNames() {
   return actualEventNames;
 };
 
+function arrayClone(arr) {
+  const copy = new Array(arr.length);
+  for (var i = 0; i < arr.length; ++i)
+    copy[i] = arr[i];
+  return copy;
+}
+
 function arrayCloneWithElement(arr, element, prepend) {
   const len = arr.length;
   const copy = new Array(len + 1);

diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js
@@ -12,7 +12,6 @@
 
   function startup() {
     const EventEmitter = NativeModule.require('events');
-    process._eventsCount = 0;
 
     const origProcProto = Object.getPrototypeOf(process);
     Object.setPrototypeOf(origProcProto, EventEmitter.prototype);
@@ -370,17 +369,17 @@
     const { kAfter, kExecutionAsyncId,
             kInitTriggerAsyncId } = async_wrap.constants;
 
-    process._fatalException = function(er) {
+    process._fatalException = function(er, shouldCatch = true) {
       var caught;
 
       // It's possible that kInitTriggerAsyncId was set for a constructor call
       // that threw and was never cleared. So clear it now.
       async_id_fields[kInitTriggerAsyncId] = 0;
 
-      if (process.domain && process.domain._errorHandler)
+      if (shouldCatch && process.domain && process.domain._errorHandler)
         caught = process.domain._errorHandler(er);
 
-      if (!caught)
+      if (shouldCatch && !caught)
         caught = process.emit('uncaughtException', er);
 
       // If someone handled it, then great.  otherwise, die in C++ land

diff --git a/lib/vm.js b/lib/vm.js
@@ -43,15 +43,15 @@ const realRunInThisContext = Script.prototype.runInThisContext;
 const realRunInContext = Script.prototype.runInContext;
 
 Script.prototype.runInThisContext = function(options) {
-  if (options && options.breakOnSigint && process._events.SIGINT) {
+  if (options && options.breakOnSigint && process.listenerCount('SIGINT')) {
     return sigintHandlersWrap(realRunInThisContext, this, [options]);
   } else {
     return realRunInThisContext.call(this, options);
   }
 };
 
 Script.prototype.runInContext = function(contextifiedSandbox, options) {
-  if (options && options.breakOnSigint && process._events.SIGINT) {
+  if (options && options.breakOnSigint && process.listenerCount('SIGINT')) {
     return sigintHandlersWrap(realRunInContext, this,
                               [contextifiedSandbox, options]);
   } else {
@@ -82,14 +82,7 @@ function createScript(code, options) {
 // Remove all SIGINT listeners and re-attach them after the wrapped function
 // has executed, so that caught SIGINT are handled by the listeners again.
 function sigintHandlersWrap(fn, thisArg, argsArray) {
-  // Using the internal list here to make sure `.once()` wrappers are used,
-  // not the original ones.
-  let sigintListeners = process._events.SIGINT;
-
-  if (Array.isArray(sigintListeners))
-    sigintListeners = sigintListeners.slice();
-  else
-    sigintListeners = [sigintListeners];
+  const sigintListeners = process.listeners('SIGINT', false);
 
   process.removeAllListeners('SIGINT');
 

diff --git a/src/async_wrap.cc b/src/async_wrap.cc
@@ -154,8 +154,7 @@ static void DestroyAsyncIdsCallback(Environment* env, void* data) {
           env->context(), Undefined(env->isolate()), 1, &async_id_value);
 
       if (ret.IsEmpty()) {
-        ClearFatalExceptionHandlers(env);
-        FatalException(env->isolate(), try_catch);
+        FatalException(env->isolate(), try_catch, false);
         UNREACHABLE();
       }
     }
@@ -175,8 +174,7 @@ void AsyncWrap::EmitPromiseResolve(Environment* env, double async_id) {
   MaybeLocal<Value> ar = fn->Call(
       env->context(), Undefined(env->isolate()), 1, &async_id_value);
   if (ar.IsEmpty()) {
-    ClearFatalExceptionHandlers(env);
-    FatalException(env->isolate(), try_catch);
+    FatalException(env->isolate(), try_catch, false);
     UNREACHABLE();
   }
 }
@@ -209,8 +207,7 @@ void AsyncWrap::EmitBefore(Environment* env, double async_id) {
   MaybeLocal<Value> ar = fn->Call(
       env->context(), Undefined(env->isolate()), 1, &async_id_value);
   if (ar.IsEmpty()) {
-    ClearFatalExceptionHandlers(env);
-    FatalException(env->isolate(), try_catch);
+    FatalException(env->isolate(), try_catch, false);
     UNREACHABLE();
   }
 }
@@ -245,8 +242,7 @@ void AsyncWrap::EmitAfter(Environment* env, double async_id) {
   MaybeLocal<Value> ar = fn->Call(
       env->context(), Undefined(env->isolate()), 1, &async_id_value);
   if (ar.IsEmpty()) {
-    ClearFatalExceptionHandlers(env);
-    FatalException(env->isolate(), try_catch);
+    FatalException(env->isolate(), try_catch, false);
     UNREACHABLE();
   }
 }
@@ -753,8 +749,7 @@ void AsyncWrap::EmitAsyncInit(Environment* env,
       env->context(), object, arraysize(argv), argv);
 
   if (ret.IsEmpty()) {
-    ClearFatalExceptionHandlers(env);
-    FatalException(env->isolate(), try_catch);
+    FatalException(env->isolate(), try_catch, false);
   }
 }
 

diff --git a/src/env.h b/src/env.h
@@ -146,7 +146,6 @@ class ModuleWrap;
   V(env_pairs_string, "envPairs")                                             \
   V(errno_string, "errno")                                                    \
   V(error_string, "error")                                                    \
-  V(events_string, "_events")                                                 \
   V(exiting_string, "_exiting")                                               \
   V(exit_code_string, "exitCode")                                             \
   V(exit_string, "exit")                                                      \

diff --git a/src/node.cc b/src/node.cc
@@ -779,28 +779,6 @@ void* ArrayBufferAllocator::Allocate(size_t size) {
 
 namespace {
 
-bool DomainHasErrorHandler(const Environment* env,
-                           const Local<Object>& domain) {
-  HandleScope scope(env->isolate());
-
-  Local<Value> domain_event_listeners_v = domain->Get(env->events_string());
-  if (!domain_event_listeners_v->IsObject())
-    return false;
-
-  Local<Object> domain_event_listeners_o =
-      domain_event_listeners_v.As<Object>();
-
-  Local<Value> domain_error_listeners_v =
-      domain_event_listeners_o->Get(env->error_string());
-
-  if (domain_error_listeners_v->IsFunction() ||
-      (domain_error_listeners_v->IsArray() &&
-      domain_error_listeners_v.As<Array>()->Length() > 0))
-    return true;
-
-  return false;
-}
-
 bool DomainsStackHasErrorHandler(const Environment* env) {
   HandleScope scope(env->isolate());
 
@@ -818,7 +796,12 @@ bool DomainsStackHasErrorHandler(const Environment* env) {
       return false;
 
     Local<Object> domain = domain_v.As<Object>();
-    if (DomainHasErrorHandler(env, domain))
+
+    Local<Value> has_error_handler = domain->Get(
+        env->context(), OneByteString(env->isolate(),
+                                      "_hasErrorListener")).ToLocalChecked();
+
+    if (has_error_handler->IsTrue())
       return true;
   }
 
@@ -2401,7 +2384,8 @@ NO_RETURN void FatalError(const char* location, const char* message) {
 
 void FatalException(Isolate* isolate,
                     Local<Value> error,
-                    Local<Message> message) {
+                    Local<Message> message,
+                    bool should_catch) {
   HandleScope scope(isolate);
 
   Environment* env = Environment::GetCurrent(isolate);
@@ -2424,9 +2408,14 @@ void FatalException(Isolate* isolate,
     // Do not call FatalException when _fatalException handler throws
     fatal_try_catch.SetVerbose(false);
 
+    Local<Value> argv[2] = {
+        error,
+        Boolean::New(env->isolate(), should_catch)
+    };
+
     // this will return true if the JS layer handled it, false otherwise
-    Local<Value> caught =
-        fatal_exception_function->Call(process_object, 1, &error);
+    Local<Value> caught = fatal_exception_function->Call(
+        env->context(), process_object, 2, argv).FromMaybe(Local<Value>());
 
     if (fatal_try_catch.HasCaught()) {
       // the fatal exception function threw, so we must exit
@@ -2449,39 +2438,38 @@ void FatalException(Isolate* isolate,
 }
 
 
-void FatalException(Isolate* isolate, const TryCatch& try_catch) {
+void FatalException(Isolate* isolate,
+                    const TryCatch& try_catch,
+                    bool should_catch) {
+  HandleScope scope(isolate);
+  if (!try_catch.IsVerbose()) {
+    FatalException(isolate,
+                   try_catch.Exception(),
+                   try_catch.Message(),
+                   should_catch);
+  }
+}
+
+
+void FatalException(Isolate* isolate,
+                    const TryCatch& try_catch) {
   HandleScope scope(isolate);
   if (!try_catch.IsVerbose()) {
-    FatalException(isolate, try_catch.Exception(), try_catch.Message());
+    FatalException(isolate,
+                   try_catch.Exception(),
+                   try_catch.Message(),
+                   true);
   }
 }
 
 
 static void OnMessage(Local<Message> message, Local<Value> error) {
   // The current version of V8 sends messages for errors only
   // (thus `error` is always set).
-  FatalException(Isolate::GetCurrent(), error, message);
+  FatalException(Isolate::GetCurrent(), error, message, true);
 }
 
 
-void ClearFatalExceptionHandlers(Environment* env) {
-  Local<Object> process = env->process_object();
-  Local<Value> events =
-      process->Get(env->context(), env->events_string()).ToLocalChecked();
-
-  if (events->IsObject()) {
-    events.As<Object>()->Set(
-        env->context(),
-        OneByteString(env->isolate(), "uncaughtException"),
-        Undefined(env->isolate())).FromJust();
-  }
-
-  process->Set(
-      env->context(),
-      env->domain_string(),
-      Undefined(env->isolate())).FromJust();
-}
-
 // Call process.emitWarning(str), fmt is a snprintf() format string
 void ProcessEmitWarning(Environment* env, const char* fmt, ...) {
   char warning[1024];
@@ -3348,12 +3336,6 @@ void SetupProcessObject(Environment* env,
   env->SetMethod(process, "_setupNextTick", SetupNextTick);
   env->SetMethod(process, "_setupPromises", SetupPromises);
   env->SetMethod(process, "_setupDomainUse", SetupDomainUse);
-
-  // pre-set _events object for faster emit checks
-  Local<Object> events_obj = Object::New(env->isolate());
-  CHECK(events_obj->SetPrototype(env->context(),
-                                 Null(env->isolate())).FromJust());
-  process->Set(env->events_string(), events_obj);
 }
 
 

diff --git a/src/node.h b/src/node.h
@@ -366,6 +366,10 @@ NODE_DEPRECATED("Use ParseEncoding(isolate, ...)",
 NODE_EXTERN void FatalException(v8::Isolate* isolate,
                                 const v8::TryCatch& try_catch);
 
+void FatalException(v8::Isolate* isolate,
+                    const v8::TryCatch& try_catch,
+                    bool should_catch);
+
 NODE_DEPRECATED("Use FatalException(isolate, ...)",
                 inline void FatalException(const v8::TryCatch& try_catch) {
   return FatalException(v8::Isolate::GetCurrent(), try_catch);

diff --git a/src/node_internals.h b/src/node_internals.h
@@ -330,11 +330,6 @@ class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
   uint32_t zero_fill_field_ = 1;  // Boolean but exposed as uint32 to JS land.
 };
 
-// Clear any domain and/or uncaughtException handlers to force the error's
-// propagation and shutdown the process. Use this to force the process to exit
-// by clearing all callbacks that could handle the error.
-void ClearFatalExceptionHandlers(Environment* env);
-
 namespace Buffer {
 v8::MaybeLocal<v8::Object> Copy(Environment* env, const char* data, size_t len);
 v8::MaybeLocal<v8::Object> New(Environment* env, size_t size);

diff --git a/src/node_url.cc b/src/node_url.cc
@@ -2183,8 +2183,7 @@ const Local<Value> URL::ToObject(Environment* env) const {
           ->Call(env->context(), undef, 9, argv);
 
   if (ret.IsEmpty()) {
-    ClearFatalExceptionHandlers(env);
-    FatalException(isolate, try_catch);
+    FatalException(isolate, try_catch, false);
   }
 
   return ret.ToLocalChecked();