From 14bc3e22f3b2c34616091183fd431f39af6c2b65 Mon Sep 17 00:00:00 2001
From: Andreas Madsen <amwebdk@gmail.com>
Date: Wed, 7 Feb 2018 20:05:45 +0100
Subject: [PATCH] domain: runtime deprecate MakeCallback

Users of MakeCallback that adds the domain property to carry context,
should start using the async_context variant of MakeCallback or the
AsyncResource class.

PR-URL: https://github.com/nodejs/node/pull/17417
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
---
 doc/api/deprecations.md                       |  9 +++++
 lib/domain.js                                 | 16 +++++++++
 .../make-callback-domain-warning/binding.cc   | 31 ++++++++++++++++
 .../make-callback-domain-warning/binding.gyp  |  9 +++++
 .../make-callback-domain-warning/test.js      | 35 +++++++++++++++++++
 5 files changed, 100 insertions(+)
 create mode 100644 test/addons/make-callback-domain-warning/binding.cc
 create mode 100644 test/addons/make-callback-domain-warning/binding.gyp
 create mode 100644 test/addons/make-callback-domain-warning/test.js

diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md
index a5386f7bf015af..1f5809624afe95 100644
--- a/doc/api/deprecations.md
+++ b/doc/api/deprecations.md
@@ -871,6 +871,15 @@ Type: Runtime
 
 `timers.unenroll()` is deprecated. Please use the publicly documented [`clearTimeout()`][] or [`clearInterval()`][] instead.
 
+<a id="DEP0097"></a>
+### DEP0097: MakeCallback with domain property
+
+Type: Runtime
+
+Users of `MakeCallback` that add the `domain` property to carry context,
+should start using the `async_context` variant of `MakeCallback` or
+`CallbackScope`, or the the high-level `AsyncResource` class.
+
 [`--pending-deprecation`]: cli.html#cli_pending_deprecation
 [`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size
 [`Buffer.from(array)`]: buffer.html#buffer_class_method_buffer_from_array
diff --git a/lib/domain.js b/lib/domain.js
index 08fbd207f171d3..be109c9ce056bd 100644
--- a/lib/domain.js
+++ b/lib/domain.js
@@ -94,13 +94,29 @@ process.setUncaughtExceptionCaptureCallback = function(fn) {
   throw err;
 };
 
+
+let sendMakeCallbackDeprecation = false;
+function emitMakeCallbackDeprecation() {
+  if (!sendMakeCallbackDeprecation) {
+    process.emitWarning(
+      'Using a domain property in MakeCallback is deprecated. Use the ' +
+      'async_context variant of MakeCallback or the AsyncResource class ' +
+      'instead.', 'DeprecationWarning', 'DEP0097');
+    sendMakeCallbackDeprecation = true;
+  }
+}
+
 function topLevelDomainCallback(cb, ...args) {
   const domain = this.domain;
+  if (exports.active && domain)
+    emitMakeCallbackDeprecation();
+
   if (domain)
     domain.enter();
   const ret = Reflect.apply(cb, this, args);
   if (domain)
     domain.exit();
+
   return ret;
 }
 
diff --git a/test/addons/make-callback-domain-warning/binding.cc b/test/addons/make-callback-domain-warning/binding.cc
new file mode 100644
index 00000000000000..c42166c7455277
--- /dev/null
+++ b/test/addons/make-callback-domain-warning/binding.cc
@@ -0,0 +1,31 @@
+#include "node.h"
+#include "v8.h"
+
+#include <assert.h>
+
+using v8::Function;
+using v8::FunctionCallbackInfo;
+using v8::Isolate;
+using v8::Local;
+using v8::Object;
+using v8::Value;
+
+namespace {
+
+void MakeCallback(const FunctionCallbackInfo<Value>& args) {
+  assert(args[0]->IsObject());
+  assert(args[1]->IsFunction());
+  Isolate* isolate = args.GetIsolate();
+  Local<Object> recv = args[0].As<Object>();
+  Local<Function> method = args[1].As<Function>();
+
+  node::MakeCallback(isolate, recv, method, 0, nullptr);
+}
+
+void Initialize(Local<Object> exports) {
+  NODE_SET_METHOD(exports, "makeCallback", MakeCallback);
+}
+
+}  // namespace
+
+NODE_MODULE(NODE_GYP_MODULE_NAME, Initialize)
diff --git a/test/addons/make-callback-domain-warning/binding.gyp b/test/addons/make-callback-domain-warning/binding.gyp
new file mode 100644
index 00000000000000..7ede63d94a0d77
--- /dev/null
+++ b/test/addons/make-callback-domain-warning/binding.gyp
@@ -0,0 +1,9 @@
+{
+  'targets': [
+    {
+      'target_name': 'binding',
+      'defines': [ 'V8_DEPRECATION_WARNINGS=1' ],
+      'sources': [ 'binding.cc' ]
+    }
+  ]
+}
diff --git a/test/addons/make-callback-domain-warning/test.js b/test/addons/make-callback-domain-warning/test.js
new file mode 100644
index 00000000000000..2ea3c3f3d14b2b
--- /dev/null
+++ b/test/addons/make-callback-domain-warning/test.js
@@ -0,0 +1,35 @@
+'use strict';
+
+const common = require('../../common');
+const assert = require('assert');
+const domain = require('domain');
+const binding = require(`./build/${common.buildType}/binding`);
+
+function makeCallback(object, cb) {
+  binding.makeCallback(object, () => setImmediate(cb));
+}
+
+let latestWarning = null;
+process.on('warning', function(warning) {
+  latestWarning = warning;
+});
+
+const d = domain.create();
+
+// When domain is disabled, no warning will be emitted
+makeCallback({ domain: d }, common.mustCall(function() {
+  assert.strictEqual(latestWarning, null);
+
+  d.run(common.mustCall(function() {
+    // No warning will be emitted when no domain property is applied
+    makeCallback({}, common.mustCall(function() {
+      assert.strictEqual(latestWarning, null);
+
+      // Warning is emitted when domain property is used and domain is enabled
+      makeCallback({ domain: d }, common.mustCall(function() {
+        assert.strictEqual(latestWarning.name, 'DeprecationWarning');
+        assert.strictEqual(latestWarning.code, 'DEP0097');
+      }));
+    }));
+  }));
+}));