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

@@ -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

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;
 }
 

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)

test/addons/make-callback-domain-warning/binding.gyp

@@ -0,0 +1,9 @@
+{
+  'targets': [
+    {
+      'target_name': 'binding',
+      'defines': [ 'V8_DEPRECATION_WARNINGS=1' ],
+      'sources': [ 'binding.cc' ]
+    }
+  ]
+}

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');
+      }));
+    }));
+  }));
+}));