Skip to content

AsyncWrap external API improvements #3461

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

Merged
trevnorris merged 3 commits into from
Nov 6, 2015
Merged

AsyncWrap external API improvements #3461

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a4e9487
async_wrap: allow some hooks to be optional
trevnorris Oct 20, 2015
80a66ba
async_wrap: new instances get uid
trevnorris Oct 19, 2015
bb1bd76
async_wrap: call callback in destructor
trevnorris Oct 20, 2015
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
Next Next commit
async_wrap: allow some hooks to be optional 
Only enforce that the init callback is passed to setupHooks(). The
remaining hooks can be optionally passed.

Throw if async_wrap.enable() runs before setting the init callback or if
setupHooks() is called while async wrap is enabled.

Add test to verify calls throw appropriately.

PR-URL: #3461
Reviewed-By: Fedor Indutny <fedor@indutny.com>
  • Loading branch information
@trevnorris
trevnorris committed Nov 6, 2015
commit a4e9487aae4297fbf400b3d0d3933a35affa557c
18 changes: 13 additions & 5 deletions src/async-wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,9 @@ RetainedObjectInfo* WrapperInfo(uint16_t class_id, Local<Value> wrapper) {

static void EnableHooksJS(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Local<Function> init_fn = env->async_hooks_init_function();
if (init_fn.IsEmpty() || !init_fn->IsFunction())
Copy link
Copy Markdown
Member

@AndreasMadsen AndreasMadsen Oct 26, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the function check required? In the reset of the code only init_fn.IsEmpty() is used to check of async-wrap is enabled. The SetupHooks function also checks that init_fn is a function.

Copy link
Copy Markdown
Contributor Author

@trevnorris trevnorris Oct 26, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently in AsyncWrap::AsyncWrap we check if the init hook is set. If not then it returns early. If this is the case then bits_ will never be set and no additional callback will ever be called for that object as a result. I can probably rethink the logic, but this seemed the most straight forward for this PR.

return env->ThrowTypeError("init callback is not assigned to a function");
env->async_hooks()->set_enable_callbacks(1);
}

Expand All @@ -116,13 +119,18 @@ static void DisableHooksJS(const FunctionCallbackInfo<Value>& args) {
static void SetupHooks(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

CHECK(args[0]->IsFunction());
CHECK(args[1]->IsFunction());
CHECK(args[2]->IsFunction());
if (env->async_hooks()->callbacks_enabled())
return env->ThrowError("hooks should not be set while also enabled");

if (!args[0]->IsFunction())
return env->ThrowTypeError("init callback must be a function");

env->set_async_hooks_init_function(args[0].As<Function>());
env->set_async_hooks_pre_function(args[1].As<Function>());
env->set_async_hooks_post_function(args[2].As<Function>());

if (args[1]->IsFunction())
env->set_async_hooks_pre_function(args[1].As<Function>());
if (args[2]->IsFunction())
env->set_async_hooks_post_function(args[2].As<Function>());
}


Expand Down
22 changes: 22 additions & 0 deletions test/parallel/test-async-wrap-throw-no-init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const async_wrap = process.binding('async_wrap');


assert.throws(function() {
async_wrap.setupHooks(null);
}, /init callback must be a function/);

assert.throws(function() {
async_wrap.enable();
}, /init callback is not assigned to a function/);

// Should not throw
async_wrap.setupHooks(() => {});
async_wrap.enable();

assert.throws(function() {
async_wrap.setupHooks(() => {});
}, /hooks should not be set while also enabled/);