domain: runtime deprecate MakeCallback #17417

AndreasMadsen · 2017-12-01T17:10:59Z

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

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

domain

AndreasMadsen · 2017-12-01T17:13:57Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11846/

/cc @addaleax @vdeturckheim

bnoordhuis · 2017-12-01T17:59:48Z

doc/api/deprecations.md

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


Back-ticks around MakeCallback() and AsyncResource?

bnoordhuis · 2017-12-01T18:03:48Z

src/env.cc

@@ -262,6 +262,12 @@ bool Environment::EmitNapiWarning() {
  return current_value;
 }

+bool Environment::EmitDomainWarning() {


Not a great name. The imperative suggests it performs an action but it's really a getter, although one with side effects (which itself is somewhat objectionable.)

At the very least call it something like ShouldEmitDomainWarning(), and look into separating the check and the update. Since you're only using it in one place, I'd split it in a getter and setter and call the setter explicitly after emitting the warning.

Hehe. Basically just copied EmitNapiWarning :D

bnoordhuis · 2017-12-01T18:04:46Z

src/node.cc

+        "Using a domain property in MakeCallback is deprecated. Use the "
+        "async_context variant of MakeCallback or the AsyncResource class "
+        "instead.",
+        "DEPXXXX");


4 space indent for line continuations and env should arguably go on a line of its own.

cjihrig · 2017-12-01T18:05:36Z

doc/api/deprecations.md

+<a id="DEPXXXX"></a>
+### MakeCallback with domain property
+
+Users of MakeCallback that adds the domain property to carry context,


that adds -> that add

cjihrig · 2017-12-01T18:06:04Z

doc/api/deprecations.md

+<a id="DEPXXXX"></a>
+### MakeCallback with domain property
+
+Users of MakeCallback that adds the domain property to carry context,


MakeCallback, domain, etc. should be in backticks I think.

AndreasMadsen · 2017-12-01T18:47:17Z

Thanks. Please check again.

bnoordhuis · 2017-12-01T19:16:01Z

src/node.cc

+        "Using a domain property in MakeCallback is deprecated. Use the "
+          "async_context variant of MakeCallback or the AsyncResource class "
+          "instead.",
+        "DEPXXXX");


Can you indent everything by 4 spaces?

addaleax · 2017-12-01T19:09:26Z

doc/api/deprecations.md

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


… or CallbackScope?

minor nit, DEP00XX in order for the tooling to catch these on release builds (there's a script that looks for the DEP00XX pattern specifically)

addaleax · 2017-12-01T19:11:32Z

src/node.cc

+
+  Local<Object> process = env->process_object();
+  MaybeLocal<Value> emit_warning = process->Get(env->context(),
+      FIXED_ONE_BYTE_STRING(env->isolate(), "emitWarning"));


I would probably move the FIXED_ONE_BYTE_STRINGs here into the list in env.h?

addaleax · 2017-12-01T19:12:44Z

src/node.cc

+  Local<Value> args[3];
+  args[0] = node::OneByteString(env->isolate(), warning);
+  args[1] = FIXED_ONE_BYTE_STRING(env->isolate(), "DeprecationWarning");
+  args[2] = node::OneByteString(env->isolate(), deprecation_code);


deprecation_code might make more sense as an internalized string (i.e. using String::NewFromOneByte with v8::NewStringType::kInternalized directly)?

addaleax · 2017-12-01T19:14:08Z

src/node.cc

+
+  // MakeCallback() unneeded, because emitWarning is internal code, it calls
+  // process.emit('warning', ..), but does so on the nextTick.
+  f.As<v8::Function>()->Call(process, arraysize(args), args);


Can you use the overload of Call() that takes a context argument?

addaleax · 2017-12-01T19:19:29Z

src/node.cc

+
+  Local<Value> f;
+
+  if (!emit_warning.ToLocal(&f)) return;


If emit_warning is empty, we should probably return immediately after that. Creating handles to strings in between the failure and returning is probably no big deal, but at some point somebody might think it’s okay to slip in code that potentially calls back into JS instead?

Either way, one issue here is that ProcessEmitDeprecationWarning has no way of signaling its caller whether it leaves an exception pending or not (i.e. if emit_warning is empty or the ->Call() below returned an empty result).

I think the options for that are:

Return Maybe<bool>, like the V8 API does, which is Just(true) on success and Nothing<bool>() on failure, and leave it for the caller to handle the issue.

Use .ToLocalChecked() on everything. The big downside is that emitting deprecation warnings with --throw-deprecation on the command line does throw an error as part of regular operations.

addaleax · 2017-12-01T19:21:26Z

src/node.cc

+      FIXED_ONE_BYTE_STRING(env->isolate(), "emitWarning"));
+
+  Local<Value> args[3];
+  args[0] = node::OneByteString(env->isolate(), warning);


UTF-8, to be a bit more future-proof? :)

AndreasMadsen · 2017-12-01T20:05:09Z

@addaleax I basically just modified the code from ProcessEmitWarning 😂 .

addaleax · 2017-12-01T20:10:35Z

@AndreasMadsen I see … 😄 The comments related to string creation are certainly optional and you can ignore them to keep parity (and/or leave a TODO comment for me 😉).

But ProcessEmitWarning() really should not do exception handling the way it does it; and at least with deprecation warnings, this becomes a real problem due to --throw-deprecation…

AndreasMadsen · 2017-12-01T21:19:42Z

@addaleax That's fair. Let's fix ProcessEmitWarning first, then I will update this PR. This PR is not exactly pressing or prone to lots of merge conflicts.

addaleax · 2017-12-01T22:19:47Z

@AndreasMadsen Just so we’re on the same page, what’s the next step? One of us opens a separate PR with those changes? Do you do that or want me to do that?

AndreasMadsen · 2017-12-01T22:26:16Z

@addaleax Yes. Go ahead and make it. I might have a few questions just to learn :)

Trott · 2017-12-01T23:04:13Z

IIRC, policy is that runtime deprecations are always treated as breaking changes so I've labeled this semver-major. That said, the TSC can opt to treat it as a non-breaking change, so feel free to put on the TSC agenda if anyone thinks it ought to land in 9.x.

BridgeAR

LGTM if a test is added

- Handle exceptions when getting `process.emitWarning` or when calling it properly - Add `Maybe<bool>` to the return type, like the V8 API uses it to indicate failure conditions - Update call sites to account for that and clean up/return to JS when encountering an error - Add an internal `ProcessEmitDeprecationWarning()` sibling for use in #17417, with common code extracted to a helper function - Allow the warning to contain non-Latin-1 characters. Since the message will usually be a template string containing data passed from the user, this is the right thing to do. - Add tests for the failure modes (except string creation failures) and UTF-8 warning messages. PR-URL: #17420 Refs: #17417 Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

jasnell · 2018-01-10T00:25:50Z

ping @AndreasMadsen ... can you add a test?

AndreasMadsen · 2018-01-10T12:41:10Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12481/

AndreasMadsen · 2018-01-10T13:28:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12485/

AndreasMadsen · 2018-01-11T15:55:19Z

@BridgeAR @jasnell I added the test. Can you approve it?

joyeecheung · 2018-01-18T13:28:29Z

Ping @AndreasMadsen this needs a rebase, thanks

AndreasMadsen · 2018-01-18T13:32:42Z

@joyeecheung Will you review it if I rebase it?

addaleax

Still LGTM, by the way.

BridgeAR

needs a rebase and my comment should be looked at.

BridgeAR · 2018-01-19T12:59:58Z

test/addons/make-callback-domain-warning/test.js

+
+  // Warning is emitted when domain property is used and domain is enabled
+  makeCallback({ domain: d }, common.mustCall(function() {
+    assert.strictEqual(latestWarning, null);


I am not certain when exactly the warning should be emitted but from the comment itself it seems to me that latestWarning should not be null. If it should be, it would be good to add a test that actually tests for notStrictEqual.

Right, this is odd

BridgeAR · 2018-01-31T02:43:02Z

Ping @AndreasMadsen

AndreasMadsen · 2018-01-31T09:36:17Z

Yeah, I was waiting for #18291 to land

BridgeAR · 2018-02-07T03:40:36Z

Ping @AndreasMadsen again. It landed 8 days ago ;-)

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

AndreasMadsen · 2018-02-07T19:08:49Z

@BridgeAR Complete rewrote the implementation, please check.

CI: https://ci.nodejs.org/job/node-test-pull-request/13018/

AndreasMadsen · 2018-02-08T09:10:43Z

fixed lint issues :D

CI: https://ci.nodejs.org/job/node-test-pull-request/13032/

BridgeAR

LGTM

AndreasMadsen · 2018-02-09T10:44:17Z

Landed in 14bc3e2

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: nodejs#17417 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 1, 2017

AndreasMadsen mentioned this pull request Dec 1, 2017

domain migration to async_hooks #17143

Open

7 tasks

bnoordhuis reviewed Dec 1, 2017

View reviewed changes

cjihrig reviewed Dec 1, 2017

View reviewed changes

vdeturckheim approved these changes Dec 1, 2017

View reviewed changes

bnoordhuis approved these changes Dec 1, 2017

View reviewed changes

cjihrig approved these changes Dec 1, 2017

View reviewed changes

addaleax reviewed Dec 1, 2017

View reviewed changes

Trott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 1, 2017

addaleax mentioned this pull request Dec 1, 2017

src: refactor and harden ProcessEmitWarning() #17420

Closed

3 tasks

jasnell approved these changes Dec 5, 2017

View reviewed changes

AndreasMadsen added the blocked PRs that are blocked by other issues or PRs. label Dec 8, 2017

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

BridgeAR reviewed Dec 28, 2017

View reviewed changes

addaleax approved these changes Jan 18, 2018

View reviewed changes

apapirovski approved these changes Jan 19, 2018

View reviewed changes

BridgeAR reviewed Jan 19, 2018

View reviewed changes

BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 19, 2018

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

domain: runtime deprecate MakeCallback

efe3b5b

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

apapirovski approved these changes Feb 7, 2018

View reviewed changes

[squash] lint

13facfa

BridgeAR approved these changes Feb 8, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 8, 2018

AndreasMadsen closed this Feb 9, 2018

domain: runtime deprecate MakeCallback #17417

domain: runtime deprecate MakeCallback #17417

Conversation

AndreasMadsen commented Dec 1, 2017

Checklist

Affected core subsystem(s)

AndreasMadsen commented Dec 1, 2017

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

AndreasMadsen commented Dec 1, 2017

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

AndreasMadsen commented Dec 1, 2017

addaleax commented Dec 1, 2017

AndreasMadsen commented Dec 1, 2017

addaleax commented Dec 1, 2017

AndreasMadsen commented Dec 1, 2017 • edited Loading

Trott commented Dec 1, 2017

BridgeAR left a comment

Choose a reason for hiding this comment

jasnell commented Jan 10, 2018

AndreasMadsen commented Jan 10, 2018

AndreasMadsen commented Jan 10, 2018

AndreasMadsen commented Jan 11, 2018

joyeecheung commented Jan 18, 2018

AndreasMadsen commented Jan 18, 2018

addaleax left a comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Jan 31, 2018

AndreasMadsen commented Jan 31, 2018

BridgeAR commented Feb 7, 2018

AndreasMadsen commented Feb 7, 2018

AndreasMadsen commented Feb 8, 2018

BridgeAR left a comment

Choose a reason for hiding this comment

AndreasMadsen commented Feb 9, 2018

AndreasMadsen commented Dec 1, 2017 •

edited

Loading

BridgeAR Jan 19, 2018 •

edited

Loading