-
Notifications
You must be signed in to change notification settings - Fork 28.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
allow passing custom async_id to node::AsyncWrap::AsyncWrap #14208
Conversation
async_id
to node::AsyncWrap::AsyncWrap
Local<Object> object, | ||
bool silent) | ||
: BaseObject(env, object), | ||
provider_type_(PROVIDER_PROMISE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this. It is not clear from the constructor signature that this is only for promises. It would be better with a unified constructor function, like how AsyncReset
is now implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree that adding another parameter to the constructor for a single case is a good idea. The general constructor is currently used in 12 locations outside of async-wrap.cc
. I only allowed AsyncWrap::AsyncReset()
the additional parameter because it's only used in a single location outside of async-wrap.cc
and only 3 locations in async-wrap.cc
. Also I think the fact that PROVIDER_PROMISE
is explicitly passed to provider_type_
makes it clear that the constructor is only meant to be used for Promises.
If everyone is absolutely against the new constructor then I'll figure out another way to have the constructor not call init()
, without needing to pass the additional argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general constructor is currently used in 12 locations outside of
async-wrap.cc
.
I can't follow that argument at all. To me, using default arguments is only an issue if there become too many of them, I don't see that happening here. Besides, your solution is just a different kind of default argument.
Also I think the fact that
PROVIDER_PROMISE
is explicitly passed toprovider_type_
makes it clear that the constructor is only meant to be used for Promises.
It is not clear from the calling code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't follow that argument at all. To me, using default arguments is only an issue if there become too many of them, I don't see that happening here.
If there's a parameter that will only ever be used by one other class, in the same file, I don't see a good reason that it should pollute the general constructor. Even if it has a default.
Besides, your solution is just a different kind of default argument.
Don't follow what you're saying.
It is not clear from the calling code.
I'm really not worried about that since it's only called once from the same file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really not worried about that since it's only called once from the same file.
For the mere mortals who didn't write the file, it is actually a really big file that and it is hard for me to orient myself when reading it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AndreasMadsen Does is not feel like a wart on the API to add a parameter with default value just to accommodate a class that isn't public? To make the intention of the alternative class constructor clear beyond the fact that PROMISE_PROVIDER
is force-ably passed I added the following to the private
class declaration:
friend class PromiseWrap;
// Constructor specifically for PromiseWrap.
AsyncWrap(Environment* env, v8::Local<v8::Object> promise, bool silent);
so there would be no ambiguity as to why that alternative constructor exists, and so no other classes could accidentally call that constructor. I believe the intention of this code is reasonably clear.
/cc @bnoordhuis if you're around, mind giving feedback on how you'd approach this scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does is not feel like a wart on the API to add a parameter with default value just to accommodate a class that isn't public?
No. At least not when it is just relaying the parameter to AsyncReset
.
I think this is about minimizing complexity and readability. I think the default parameter is simultaneously a more simple solution and more readable.
I'm not strong enough in C++ to give a better argument and "wart" is not concrete enough for me to argue against. I will let others take the argument from here.
/cc @nodejs/async_hooks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is about minimizing complexity and readability. I think the default parameter is simultaneously a more simple solution and more readable.
At this point I think we just have a different aesthetic. IMO it's more clear to have a second documented constructor dedicated to a singular edge case usage than use an additional optional parameter.
The key here is that in no foreseeable case will the optional parameter be used by anything other than PromiseWrap
. If there were even the slightest chance of that possibility then I'd agree that adding the optional parameter would be the correct approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO an adding a comment stating that this ctor is for use by PromiseWrap
only, in order not to expose a rarely used argument, is a decent compromise.
src/async-wrap.h
Outdated
@@ -117,7 +117,7 @@ class AsyncWrap : public BaseObject { | |||
|
|||
inline double get_trigger_id() const; | |||
|
|||
void AsyncReset(bool silent = false); | |||
void AsyncReset(double eid = 0, bool silent = false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use -1
as the value to check, this is also how the triggerAsyncId
works in the public API. 0
as an asyncId
has another meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. There are benefits for each case, but I was on the fence about which to always default to. If everyone else thinks -1
is the better value then I'll be happy to go with that.
How does the CI addition work? Will it always run or is there a special job? Also, we need to make sure it runs on Windows too. Let's not repeat that mistake :) |
The additional |
Vaguely apropo #14241 |
Where do we stand here? Is there any conclusion? And this needs a rebase. |
@BridgeAR Forgot about this. going to rebase and retest. |
c4796a3
to
b2865ba
Compare
@@ -434,6 +436,14 @@ test-timers-clean: | |||
test-async-hooks: | |||
$(PYTHON) tools/test.py --mode=release async-hooks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$(PYTHON) tools/test.py --mode=release $(CI_ASYNC_HOOKS)
?
Local<Object> object, | ||
bool silent) | ||
: BaseObject(env, object), | ||
provider_type_(PROVIDER_PROMISE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO an adding a comment stating that this ctor is for use by PromiseWrap
only, in order not to expose a rarely used argument, is a decent compromise.
src/async-wrap.cc
Outdated
@@ -428,7 +428,8 @@ void AsyncWrap::ClearIdStack(const FunctionCallbackInfo<Value>& args) { | |||
void AsyncWrap::AsyncReset(const FunctionCallbackInfo<Value>& args) { | |||
AsyncWrap* wrap; | |||
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); | |||
wrap->AsyncReset(); | |||
double eid = args[0]->IsNumber() ? args[0]->NumberValue() : -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use a more verbose name instead of eid
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but execution_async_id
is a bit long. would exec_id
be good enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exec_id
is great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imho, execution_async_id
is great, too – let’s choose verbose names whenever we can, a lot more people need to understand rather the code than need to write it.
@@ -5,6 +5,11 @@ const assert = require('assert'); | |||
const async_hooks = require('async_hooks'); | |||
const binding = require(`./build/${common.buildType}/binding`); | |||
|
|||
if (process.env.NODE_TEST_WITH_ASYNC_HOOKS) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the tradeoffs of this VS doing delete process.env.NODE_TEST_WITH_ASYNC_HOOKS
before L3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean. Mind clarifying?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we delete NODE_TEST_WITH_ASYNC_HOOKS
before the L3 (const common = require('../../common');
) it will not run the hooking logic of the harness https://github.com/nodejs/node/blob/b2865ba8a3330f763e8a3833a82bef24660de916/test/common/index.js#L64-L65
Will that make this testable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while the possibility does exist, we depend on env vars existing for other things that could also be deleted. So I'm not going to worry about it.
b2865ba
to
e6cdef9
Compare
@refack Comments (except for |
The new test-with-async-hooks runs all normal tests (except async-hooks) with the environment variable NODE_TEST_WITH_ASYNC_HOOKS set. These extra checks do a minimum check to make sure async_hooks operates normally under all other tests. e.g. if init() or destroy() is called twice for the same id. Also move test "async-hooks" from CI_JS_SUITES into its own CI_ASYNC_HOOKS. Makes it cleaner to add, instead of supplying a massive list of tests that may change in the future. PR-URL: nodejs#14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
When running tests with NODE_TEST_WITH_ASYNC_HOOKS and the same asyncId is detected twice print the stack traces of both init() calls. Also print if the resource is the same instance. PR-URL: nodejs#14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
The test addons/async-hooks-promise depends on there being only one hook available. So skip it if NODE_TEST_WITH_ASYNC_HOOKS is set. PR-URL: nodejs#14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Another optional argument is about to be added to AsyncWrap. So instead of piling them on, create a separate constructor specifically for PromiseWrap since it's the only class that uses the "silent" argument. PR-URL: nodejs#14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Allow the user to pass in an execution_async_id instead of always generating one. This way the JS API can be used to pre-allocate the execution_async_id when the JS object is instantiated, before the native resource is created. Also allow the new execution_async_id to be passed via asyncReset(). PR-URL: nodejs#14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
e6cdef9
to
2b9b46c
Compare
The new test-with-async-hooks runs all normal tests (except async-hooks) with the environment variable NODE_TEST_WITH_ASYNC_HOOKS set. These extra checks do a minimum check to make sure async_hooks operates normally under all other tests. e.g. if init() or destroy() is called twice for the same id. Also move test "async-hooks" from CI_JS_SUITES into its own CI_ASYNC_HOOKS. Makes it cleaner to add, instead of supplying a massive list of tests that may change in the future. PR-URL: #14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
When running tests with NODE_TEST_WITH_ASYNC_HOOKS and the same asyncId is detected twice print the stack traces of both init() calls. Also print if the resource is the same instance. PR-URL: #14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
The test addons/async-hooks-promise depends on there being only one hook available. So skip it if NODE_TEST_WITH_ASYNC_HOOKS is set. PR-URL: #14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Another optional argument is about to be added to AsyncWrap. So instead of piling them on, create a separate constructor specifically for PromiseWrap since it's the only class that uses the "silent" argument. PR-URL: #14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Allow the user to pass in an execution_async_id instead of always generating one. This way the JS API can be used to pre-allocate the execution_async_id when the JS object is instantiated, before the native resource is created. Also allow the new execution_async_id to be passed via asyncReset(). PR-URL: nodejs#14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
The new test-with-async-hooks runs all normal tests (except async-hooks) with the environment variable NODE_TEST_WITH_ASYNC_HOOKS set. These extra checks do a minimum check to make sure async_hooks operates normally under all other tests. e.g. if init() or destroy() is called twice for the same id. Also move test "async-hooks" from CI_JS_SUITES into its own CI_ASYNC_HOOKS. Makes it cleaner to add, instead of supplying a massive list of tests that may change in the future. PR-URL: #14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
When running tests with NODE_TEST_WITH_ASYNC_HOOKS and the same asyncId is detected twice print the stack traces of both init() calls. Also print if the resource is the same instance. PR-URL: #14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
The test addons/async-hooks-promise depends on there being only one hook available. So skip it if NODE_TEST_WITH_ASYNC_HOOKS is set. PR-URL: #14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Another optional argument is about to be added to AsyncWrap. So instead of piling them on, create a separate constructor specifically for PromiseWrap since it's the only class that uses the "silent" argument. PR-URL: #14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Allow the user to pass in an execution_async_id instead of always generating one. This way the JS API can be used to pre-allocate the execution_async_id when the JS object is instantiated, before the native resource is created. Also allow the new execution_async_id to be passed via asyncReset(). PR-URL: #14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
The new test-with-async-hooks runs all normal tests (except async-hooks) with the environment variable NODE_TEST_WITH_ASYNC_HOOKS set. These extra checks do a minimum check to make sure async_hooks operates normally under all other tests. e.g. if init() or destroy() is called twice for the same id. Also move test "async-hooks" from CI_JS_SUITES into its own CI_ASYNC_HOOKS. Makes it cleaner to add, instead of supplying a massive list of tests that may change in the future. PR-URL: nodejs/node#14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
When running tests with NODE_TEST_WITH_ASYNC_HOOKS and the same asyncId is detected twice print the stack traces of both init() calls. Also print if the resource is the same instance. PR-URL: nodejs/node#14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
The test addons/async-hooks-promise depends on there being only one hook available. So skip it if NODE_TEST_WITH_ASYNC_HOOKS is set. PR-URL: nodejs/node#14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Another optional argument is about to be added to AsyncWrap. So instead of piling them on, create a separate constructor specifically for PromiseWrap since it's the only class that uses the "silent" argument. PR-URL: nodejs/node#14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Allow the user to pass in an execution_async_id instead of always generating one. This way the JS API can be used to pre-allocate the execution_async_id when the JS object is instantiated, before the native resource is created. Also allow the new execution_async_id to be passed via asyncReset(). PR-URL: nodejs/node#14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Allow the user to pass in an execution_async_id instead of always generating one. This way the JS API can be used to pre-allocate the execution_async_id when the JS object is instantiated, before the native resource is created. Also allow the new execution_async_id to be passed via asyncReset(). PR-URL: nodejs/node#14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
The new test-with-async-hooks runs all normal tests (except async-hooks) with the environment variable NODE_TEST_WITH_ASYNC_HOOKS set. These extra checks do a minimum check to make sure async_hooks operates normally under all other tests. e.g. if init() or destroy() is called twice for the same id. Also move test "async-hooks" from CI_JS_SUITES into its own CI_ASYNC_HOOKS. Makes it cleaner to add, instead of supplying a massive list of tests that may change in the future. PR-URL: #14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
When running tests with NODE_TEST_WITH_ASYNC_HOOKS and the same asyncId is detected twice print the stack traces of both init() calls. Also print if the resource is the same instance. PR-URL: #14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
The test addons/async-hooks-promise depends on there being only one hook available. So skip it if NODE_TEST_WITH_ASYNC_HOOKS is set. PR-URL: #14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Another optional argument is about to be added to AsyncWrap. So instead of piling them on, create a separate constructor specifically for PromiseWrap since it's the only class that uses the "silent" argument. PR-URL: #14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Allow the user to pass in an execution_async_id instead of always generating one. This way the JS API can be used to pre-allocate the execution_async_id when the JS object is instantiated, before the native resource is created. Also allow the new execution_async_id to be passed via asyncReset(). PR-URL: #14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
async_wrap, async_hooks
make test-with-async-hooks
that adds the environment variableNODE_TEST_WITH_ASYNC_HOOKS
and skips theasync-hooks
test suite.NODE_TEST_WITH_ASYNC_HOOKS
AsyncWrap
constructor specifically forPromiseWrap
, since it's a special case, to prevent constantly needing to add new arguments to the general constructorasync_id
tonode::AsyncWrap::AsyncWrap
that is used, instead of generating a new one. Also allows passing anasync_id
in viaasyncReset()
. (note: this doesn't include a change toAsyncResource
inlib/async_hooks.js
because want to have discussion on argument ordering)IMPORTANT: Only the API to allow passing a new
async_id
has been added, but hasn't been implemented yet because of various issues. Next I will create a tracking issue with all cases, and that link will be added here after it's posted.CI: https://ci.nodejs.org/job/node-test-pull-request/9101/
UPDATE: Tracking issue for edge cases using this API: #14209