Skip to content
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.

Sign up for GitHub

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

Jump to bottom

allow passing custom async_id to node::AsyncWrap::AsyncWrap #14208

Merged
merged 5 commits into from
Sep 27, 2017
Merged

allow passing custom async_id to node::AsyncWrap::AsyncWrap #14208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4321206
build: add test-with-async-hooks
trevnorris Jun 20, 2017
44dc449
test: print resource stack on error
trevnorris Jun 16, 2017
d28d20c
test: skip test when checking async_hooks
trevnorris Jun 20, 2017
e1eae3c
async_wrap: add constructor for PromiseWrap
trevnorris Jun 14, 2017
2b9b46c
async_wrap: allow user to pass execution_async_id
trevnorris Jun 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ test: all
$(MAKE) build-addons-napi
$(MAKE) cctest
$(PYTHON) tools/test.py --mode=release -J \
$(CI_ASYNC_HOOKS) \
$(CI_JS_SUITES) \
$(CI_NATIVE_SUITES)
$(MAKE) lint
Expand Down Expand Up @@ -334,7 +335,8 @@ test-all-valgrind: test-build
$(PYTHON) tools/test.py --mode=debug,release --valgrind

CI_NATIVE_SUITES := addons addons-napi
CI_JS_SUITES := abort async-hooks doctool inspector known_issues message parallel pseudo-tty sequential
CI_ASYNC_HOOKS := async-hooks
CI_JS_SUITES := abort doctool inspector known_issues message parallel pseudo-tty sequential

# Build and test addons without building anything else
test-ci-native: LOGLEVEL := info
Expand All @@ -347,7 +349,7 @@ test-ci-native: | test/addons/.buildstamp test/addons-napi/.buildstamp
test-ci-js: | clear-stalled
$(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \
--mode=release --flaky-tests=$(FLAKY_TESTS) \
$(TEST_CI_ARGS) $(CI_JS_SUITES)
$(TEST_CI_ARGS) $(CI_ASYNC_HOOKS) $(CI_JS_SUITES)
# Clean up any leftover processes, error if found.
ps awwx | grep Release/node | grep -v grep | cat
@PS_OUT=`ps awwx | grep Release/node | grep -v grep | awk '{print $$1}'`; \
Expand All @@ -360,7 +362,7 @@ test-ci: | clear-stalled build-addons build-addons-napi
out/Release/cctest --gtest_output=tap:cctest.tap
$(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \
--mode=release --flaky-tests=$(FLAKY_TESTS) \
$(TEST_CI_ARGS) $(CI_JS_SUITES) $(CI_NATIVE_SUITES)
$(TEST_CI_ARGS) $(CI_ASYNC_HOOKS) $(CI_JS_SUITES) $(CI_NATIVE_SUITES)
# Clean up any leftover processes, error if found.
ps awwx | grep Release/node | grep -v grep | cat
@PS_OUT=`ps awwx | grep Release/node | grep -v grep | awk '{print $$1}'`; \
Expand Down Expand Up @@ -434,6 +436,14 @@ test-timers-clean:
test-async-hooks:
$(PYTHON) tools/test.py --mode=release async-hooks
Copy link
Contributor

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)?


test-with-async-hooks:
$(MAKE) build-addons
$(MAKE) build-addons-napi
$(MAKE) cctest
NODE_TEST_WITH_ASYNC_HOOKS=1 $(PYTHON) tools/test.py --mode=release -J \
$(CI_JS_SUITES) \
$(CI_NATIVE_SUITES)


ifneq ("","$(wildcard deps/v8/tools/run-tests.py)")
test-v8: v8
Expand Down
30 changes: 24 additions & 6 deletions src/async-wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ void AsyncWrap::EmitAfter(Environment* env, double async_id) {
class PromiseWrap : public AsyncWrap {
public:
PromiseWrap(Environment* env, Local<Object> object, bool silent)
: AsyncWrap(env, object, PROVIDER_PROMISE, silent) {
: AsyncWrap(env, object, silent) {
MakeWeak(this);
}
size_t self_size() const override { return sizeof(*this); }
Expand Down Expand Up @@ -451,7 +451,8 @@ void AsyncWrap::ClearAsyncIdStack(const FunctionCallbackInfo<Value>& args) {
void AsyncWrap::AsyncReset(const FunctionCallbackInfo<Value>& args) {
AsyncWrap* wrap;
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
wrap->AsyncReset();
double execution_async_id = args[0]->IsNumber() ? args[0]->NumberValue() : -1;
wrap->AsyncReset(execution_async_id);
}


Expand Down Expand Up @@ -574,7 +575,7 @@ void LoadAsyncWrapperInfo(Environment* env) {
AsyncWrap::AsyncWrap(Environment* env,
Local<Object> object,
ProviderType provider,
bool silent)
double execution_async_id)
: BaseObject(env, object),
provider_type_(provider) {
CHECK_NE(provider, PROVIDER_NONE);
Expand All @@ -584,7 +585,23 @@ AsyncWrap::AsyncWrap(Environment* env,
persistent().SetWrapperClassId(NODE_ASYNC_ID_OFFSET + provider);

// Use AsyncReset() call to execute the init() callbacks.
AsyncReset(silent);
AsyncReset(execution_async_id);
}


// This is specifically used by the PromiseWrap constructor.
AsyncWrap::AsyncWrap(Environment* env,
Local<Object> object,
bool silent)
: BaseObject(env, object),
provider_type_(PROVIDER_PROMISE) {
Copy link
Member

@AndreasMadsen AndreasMadsen Jul 13, 2017 •
edited
Loading

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.

Copy link
Contributor Author

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.

Copy link
Member

@AndreasMadsen AndreasMadsen Jul 13, 2017 •
edited
Loading

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 to provider_type_ makes it clear that the constructor is only meant to be used for Promises.

It is not clear from the calling code.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor

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.

CHECK_GE(object->InternalFieldCount(), 1);

// Shift provider value over to prevent id collision.
persistent().SetWrapperClassId(NODE_ASYNC_ID_OFFSET + provider_type_);

// Use AsyncReset() call to execute the init() callbacks.
AsyncReset(-1, silent);
}


Expand All @@ -596,8 +613,9 @@ AsyncWrap::~AsyncWrap() {
// Generalized call for both the constructor and for handles that are pooled
// and reused over their lifetime. This way a new uid can be assigned when
// the resource is pulled out of the pool and put back into use.
void AsyncWrap::AsyncReset(bool silent) {
async_id_ = env()->new_async_id();
void AsyncWrap::AsyncReset(double execution_async_id, bool silent) {
async_id_ =
execution_async_id == -1 ? env()->new_async_id() : execution_async_id;
trigger_async_id_ = env()->get_init_trigger_async_id();

if (silent) return;
Expand Down
8 changes: 6 additions & 2 deletions src/async-wrap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class AsyncWrap : public BaseObject {
AsyncWrap(Environment* env,
v8::Local<v8::Object> object,
ProviderType provider,
bool silent = false);
double execution_async_id = -1);

virtual ~AsyncWrap();

Expand Down Expand Up @@ -133,7 +133,7 @@ class AsyncWrap : public BaseObject {

inline double get_trigger_async_id() const;

void AsyncReset(bool silent = false);
void AsyncReset(double execution_async_id = -1, bool silent = false);

// Only call these within a valid HandleScope.
v8::MaybeLocal<v8::Value> MakeCallback(const v8::Local<v8::Function> cb,
Expand All @@ -150,6 +150,10 @@ class AsyncWrap : public BaseObject {
virtual size_t self_size() const = 0;

private:
friend class PromiseWrap;

// This is specifically used by the PromiseWrap constructor.
AsyncWrap(Environment* env, v8::Local<v8::Object> promise, bool silent);
inline AsyncWrap();
const ProviderType provider_type_;
// Because the values may be Reset(), cannot be made const.
Expand Down
5 changes: 5 additions & 0 deletions test/addons/async-hooks-promise/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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?

Copy link
Contributor Author

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.

common.skip('cannot test with env var NODE_TEST_WITH_ASYNC_HOOKS');
return;
}

// Baseline to make sure the internal field isn't being set.
assert.strictEqual(
binding.getPromiseField(Promise.resolve(1)),
Expand Down
9 changes: 6 additions & 3 deletions test/common/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,18 +79,21 @@ if (process.env.NODE_TEST_WITH_ASYNC_HOOKS) {
if (destroyListList[id] !== undefined) {
process._rawDebug(destroyListList[id]);
process._rawDebug();
throw new Error(`same id added twice (${id})`);
throw new Error(`same id added to destroy list twice (${id})`);
}
destroyListList[id] = new Error().stack;
_queueDestroyAsyncId(id);
};

require('async_hooks').createHook({
init(id, ty, tr, h) {
init(id, ty, tr, r) {
if (initHandles[id]) {
process._rawDebug(
`Is same resource: ${r === initHandles[id].resource}`);
process._rawDebug(`Previous stack:\n${initHandles[id].stack}\n`);
throw new Error(`init called twice for same id (${id})`);
}
initHandles[id] = h;
initHandles[id] = { resource: r, stack: new Error().stack.substr(6) };
},
before() { },
after() { },
Expand Down