From 009a921f64aa0202c10b9e1e14e583dcae3a7446 Mon Sep 17 00:00:00 2001
From: Trevor Norris <trev.norris@gmail.com>
Date: Wed, 9 Aug 2017 00:02:20 -0600
Subject: [PATCH] async_wrap: return undefined if domain is disposed

v8::MaybeLocal::ToLocalChecked() will abort on an empty handle.
AsyncWrap::MakeCallback returns an empty handle if the domain is
disposed, but this should not cause the process to abort. So instead
return v8::Undefined() and allow the error to be handled normally.

PR-URL: https://github.com/nodejs/node/pull/14722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
---
 src/async-wrap.cc                             |  5 ++--
 .../test-domain-abort-when-disposed.js        | 25 +++++++++++++++++++
 2 files changed, 28 insertions(+), 2 deletions(-)
 create mode 100644 test/parallel/test-domain-abort-when-disposed.js

diff --git a/src/async-wrap.cc b/src/async-wrap.cc
index 3228299233e468..8f513ba4524795 100644
--- a/src/async-wrap.cc
+++ b/src/async-wrap.cc
@@ -54,6 +54,7 @@ using v8::String;
 using v8::Symbol;
 using v8::TryCatch;
 using v8::Uint32Array;
+using v8::Undefined;
 using v8::Value;
 
 using AsyncHooks = node::Environment::AsyncHooks;
@@ -696,7 +697,7 @@ MaybeLocal<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
                                                 get_trigger_id());
 
   if (!PreCallbackExecution(this, true)) {
-    return MaybeLocal<Value>();
+    return Undefined(env()->isolate());
   }
 
   // Finally... Get to running the user's callback.
@@ -707,7 +708,7 @@ MaybeLocal<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
   }
 
   if (!PostCallbackExecution(this, true)) {
-    return Local<Value>();
+    return Undefined(env()->isolate());
   }
 
   exec_scope.Dispose();
diff --git a/test/parallel/test-domain-abort-when-disposed.js b/test/parallel/test-domain-abort-when-disposed.js
new file mode 100644
index 00000000000000..3a02b1e94c1b11
--- /dev/null
+++ b/test/parallel/test-domain-abort-when-disposed.js
@@ -0,0 +1,25 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const domain = require('domain');
+
+// These were picked arbitrarily and are only used to trigger arync_hooks.
+const JSStream = process.binding('js_stream').JSStream;
+const Socket = require('net').Socket;
+
+const handle = new JSStream();
+handle.domain = domain.create();
+handle.domain.dispose();
+
+handle.close = () => {};
+handle.isAlive = () => { throw new Error(); };
+
+const s = new Socket({ handle });
+
+// When AsyncWrap::MakeCallback() returned an empty handle the
+// MaybeLocal::ToLocalChecked() call caused the process to abort. By returning
+// v8::Undefined() it allows the error to propagate to the 'error' event.
+s.on('error', common.mustCall((e) => {
+  assert.strictEqual(e.code, 'EINVAL');
+}));