Skip to content

Begin AsyncWrap maintenance #3139

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 6 additions & 6 deletions lib/dns.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const uv = process.binding('uv');

const GetAddrInfoReqWrap = cares.GetAddrInfoReqWrap;
const GetNameInfoReqWrap = cares.GetNameInfoReqWrap;
const QueryReqWrap = cares.QueryReqWrap;

const isIp = net.isIP;

Expand Down Expand Up @@ -223,12 +224,11 @@ function resolver(bindingName) {
}

callback = makeAsync(callback);
var req = {
bindingName: bindingName,
callback: callback,
hostname: name,
oncomplete: onresolve
};
var req = new QueryReqWrap();
req.bindingName = bindingName;
req.callback = callback;
req.hostname = name;
req.oncomplete = onresolve;
var err = binding(req, name);
if (err) throw errnoException(err, bindingName);
callback.immediately = true;
Expand Down
9 changes: 5 additions & 4 deletions src/async-wrap-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,11 @@ inline AsyncWrap::AsyncWrap(Environment* env,
ProviderType provider,
AsyncWrap* parent)
: BaseObject(env, object), bits_(static_cast<uint32_t>(provider) << 1) {
// Only set wrapper class id if object will be Wrap'd.
if (object->InternalFieldCount() > 0)
// Shift provider value over to prevent id collision.
persistent().SetWrapperClassId(NODE_ASYNC_ID_OFFSET + provider);
CHECK_NE(provider, PROVIDER_NONE);
CHECK_GE(object->InternalFieldCount(), 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

These two will throw errors. Doesn't it make it major change? If this is an internal function, do we really need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't throw an error. It aborts. The first couldn't have happened anyway because of how the function signature works. It's there as a sanity check for future development. The latter should never have been happening as it would have caused issues when iterating the heap.


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

// Check user controlled flag to see if the init callback should run.
if (!env->using_asyncwrap())
Expand Down
6 changes: 3 additions & 3 deletions src/async-wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,26 +12,26 @@ namespace node {

#define NODE_ASYNC_PROVIDER_TYPES(V) \
V(NONE) \
V(CARES) \
V(CONNECTWRAP) \
V(CRYPTO) \
V(FSEVENTWRAP) \
V(FSREQWRAP) \
V(GETADDRINFOREQWRAP) \
V(GETNAMEINFOREQWRAP) \
V(JSSTREAM) \
V(PIPEWRAP) \
V(PIPECONNECTWRAP) \
V(PROCESSWRAP) \
V(QUERYWRAP) \
V(REQWRAP) \
V(SHUTDOWNWRAP) \
V(SIGNALWRAP) \
V(STATWATCHER) \
V(TCPWRAP) \
V(TCPCONNECTWRAP) \
V(TIMERWRAP) \
V(TLSWRAP) \
V(TTYWRAP) \
V(UDPWRAP) \
V(UDPSENDWRAP) \
V(WRITEWRAP) \
V(ZLIB)

Expand Down
13 changes: 13 additions & 0 deletions src/cares_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,11 @@ static void NewGetNameInfoReqWrap(const FunctionCallbackInfo<Value>& args) {
}


static void NewQueryReqWrap(const FunctionCallbackInfo<Value>& args) {
CHECK(args.IsConstructCall());
}


static int cmp_ares_tasks(const ares_task_t* a, const ares_task_t* b) {
if (a->sock < b->sock)
return -1;
Expand Down Expand Up @@ -1312,6 +1317,14 @@ static void Initialize(Local<Object> target,
FIXED_ONE_BYTE_STRING(env->isolate(), "GetNameInfoReqWrap"));
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "GetNameInfoReqWrap"),
niw->GetFunction());

Local<FunctionTemplate> qrw =
FunctionTemplate::New(env->isolate(), NewQueryReqWrap);
qrw->InstanceTemplate()->SetInternalFieldCount(1);
qrw->SetClassName(
FIXED_ONE_BYTE_STRING(env->isolate(), "QueryReqWrap"));
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "QueryReqWrap"),
qrw->GetFunction());
}

} // namespace cares_wrap
Expand Down
13 changes: 13 additions & 0 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,13 @@ inline Environment::Environment(v8::Local<v8::Context> context,
set_as_external(v8::External::New(isolate(), this));
set_binding_cache_object(v8::Object::New(isolate()));
set_module_load_list_array(v8::Array::New(isolate()));

v8::Local<v8::FunctionTemplate> fn_t = v8::FunctionTemplate::New(isolate());
fn_t->SetClassName(FIXED_ONE_BYTE_STRING(isolate(), "InternalFieldObject"));
v8::Local<v8::ObjectTemplate> obj_t = fn_t->InstanceTemplate();
obj_t->SetInternalFieldCount(1);
set_generic_internal_field_template(obj_t);
Copy link
Member

Choose a reason for hiding this comment

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

Mild cognitive dissonance here, the _t suffix makes it look like the variables are typedefs at a quick glance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot that convention. Was using it as shorthand for template. Will change.


RB_INIT(&cares_task_list_);
handle_cleanup_waiting_ = 0;
}
Expand Down Expand Up @@ -513,6 +520,12 @@ inline void Environment::SetTemplateMethod(v8::Local<v8::FunctionTemplate> that,
function->SetName(name_string); // NODE_SET_METHOD() compatibility.
}

inline v8::Local<v8::Object> Environment::NewInternalFieldObject() {
v8::MaybeLocal<v8::Object> m_obj =
generic_internal_field_template()->NewInstance(this->context());
Copy link
Member

Choose a reason for hiding this comment

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

The this-> isn't necessary, is it? It immediately makes me scan for a local variable context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope it's not. Must not have mentally context switched from JS.

return m_obj.ToLocalChecked();
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we need to check if the MayBe objects are empty before using them. What does ToLocalChecked do? Couldn't find much info on v8 docs.

Edit: I guess it internally checks if it is empty and returns the Local object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It aborts if an empty object is returned. There's no point in checking before because there's no recourse if it is.

}

#define V(PropertyName, StringValue) \
inline \
v8::Local<v8::String> Environment::IsolateData::PropertyName() const { \
Expand Down
2 changes: 2 additions & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,7 @@ namespace node {
V(context, v8::Context) \
V(domain_array, v8::Array) \
V(fs_stats_constructor_function, v8::Function) \
V(generic_internal_field_template, v8::ObjectTemplate) \
V(jsstream_constructor_template, v8::FunctionTemplate) \
V(module_load_list_array, v8::Array) \
V(pipe_constructor_template, v8::FunctionTemplate) \
Expand Down Expand Up @@ -488,6 +489,7 @@ class Environment {
const char* name,
v8::FunctionCallback callback);

inline v8::Local<v8::Object> NewInternalFieldObject();

// Strings are shared across shared contexts. The getters simply proxy to
// the per-isolate primitive.
Expand Down
6 changes: 4 additions & 2 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4624,6 +4624,7 @@ class PBKDF2Request : public AsyncWrap {
iter_(iter) {
if (key() == nullptr)
FatalError("node::PBKDF2Request()", "Out of Memory");
Wrap(object, this);
}

~PBKDF2Request() override {
Expand Down Expand Up @@ -4833,7 +4834,7 @@ void PBKDF2(const FunctionCallbackInfo<Value>& args) {
digest = EVP_sha1();
}

obj = Object::New(env->isolate());
obj = env->NewInternalFieldObject();
req = new PBKDF2Request(env,
obj,
digest,
Expand Down Expand Up @@ -4885,6 +4886,7 @@ class RandomBytesRequest : public AsyncWrap {
data_(static_cast<char*>(malloc(size))) {
if (data() == nullptr)
FatalError("node::RandomBytesRequest()", "Out of Memory");
Wrap(object, this);
}

~RandomBytesRequest() override {
Expand Down Expand Up @@ -5001,7 +5003,7 @@ void RandomBytes(const FunctionCallbackInfo<Value>& args) {
if (size < 0 || size > Buffer::kMaxLength)
return env->ThrowRangeError("size is not a valid Smi");

Local<Object> obj = Object::New(env->isolate());
Local<Object> obj = env->NewInternalFieldObject();
RandomBytesRequest* req = new RandomBytesRequest(env, obj, size);

if (args[1]->IsFunction()) {
Expand Down
2 changes: 1 addition & 1 deletion src/pipe_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class PipeConnectWrap : public ReqWrap<uv_connect_t> {


PipeConnectWrap::PipeConnectWrap(Environment* env, Local<Object> req_wrap_obj)
: ReqWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_PIPEWRAP) {
: ReqWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_PIPECONNECTWRAP) {
Wrap(req_wrap_obj, this);
}

Expand Down
2 changes: 1 addition & 1 deletion src/tcp_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class TCPConnectWrap : public ReqWrap<uv_connect_t> {


TCPConnectWrap::TCPConnectWrap(Environment* env, Local<Object> req_wrap_obj)
: ReqWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_TCPWRAP) {
: ReqWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_TCPCONNECTWRAP) {
Wrap(req_wrap_obj, this);
}

Expand Down
2 changes: 1 addition & 1 deletion src/udp_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class SendWrap : public ReqWrap<uv_udp_send_t> {
SendWrap::SendWrap(Environment* env,
Local<Object> req_wrap_obj,
bool have_callback)
: ReqWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_UDPWRAP),
: ReqWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_UDPSENDWRAP),
have_callback_(have_callback) {
Wrap(req_wrap_obj, this);
}
Expand Down
103 changes: 103 additions & 0 deletions test/parallel/test-async-wrap-check-providers.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const crypto = require('crypto');
const dgram = require('dgram');
const dns = require('dns');
const fs = require('fs');
const net = require('net');
const tls = require('tls');
const zlib = require('zlib');
const ChildProcess = require('child_process').ChildProcess;
const StreamWrap = require('_stream_wrap').StreamWrap;
const async_wrap = process.binding('async_wrap');
const pkeys = Object.keys(async_wrap.Providers);

const keyList = pkeys.slice();
// Drop NONE
keyList.splice(0, 1);


function init(id) {
const idx = keyList.indexOf(pkeys[id]);
if (idx === -1)
return;
keyList.splice(idx, 1);
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd simplify this to pkeys = pkeys.filter(e => e != id) (and change pkeys from const to var.)

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'm fine with the change, but also curious why you find it preferable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, I don't understand this change. it does a numeric comparison against a string comparison. For it to work similarly to how it should work before the change would have to be:

function init(id) {
  keyList = keyList.filter(e => e != pkeys[id]);
}

Though still curious to why this approach is preferred.

Copy link
Member

Choose a reason for hiding this comment

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

The functional approach is generally easier to read than the imperative one, IMO. .filter() tells you what it does, .splice() how it does it.


function noop() { }

async_wrap.setupHooks(init, noop, noop);

async_wrap.enable();


setTimeout(function() { });

fs.stat(__filename, noop);
fs.watchFile(__filename, noop);
fs.unwatchFile(__filename);
fs.watch(__filename).close();

dns.lookup('localhost', noop);
dns.lookupService('::', 0, noop);
dns.resolve('localhost', noop);

new StreamWrap(new net.Socket());

new (process.binding('tty_wrap').TTY)();

crypto.randomBytes(1, noop);

try {
fs.unlinkSync(common.PIPE);
} catch(e) { }

net.createServer(function(c) {
c.end();
this.close();
}).listen(common.PIPE, function() {
net.connect(common.PIPE, noop);
});

net.createServer(function(c) {
c.end();
this.close(checkTLS);
}).listen(common.PORT, function() {
net.connect(common.PORT, noop);
});

dgram.createSocket('udp4').bind(common.PORT, function() {
this.send(new Buffer(2), 0, 2, common.PORT, '::', () => {
this.close();
});
});

process.on('SIGINT', () => process.exit());

// Run from closed net server above.
function checkTLS() {
let options = {
key: fs.readFileSync(common.fixturesDir + '/keys/ec-key.pem'),
cert: fs.readFileSync(common.fixturesDir + '/keys/ec-cert.pem')
};
let server = tls.createServer(options, noop).listen(common.PORT, function() {
tls.connect(common.PORT, { rejectUnauthorized: false }, function() {
this.destroy();
server.close();
});
});
}

zlib.createGzip();

new ChildProcess();

process.on('exit', function() {
if (keyList.length !== 0) {
process._rawDebug('Not all keys have been used:');
process._rawDebug(keyList);
assert.equal(keyList.length, 0);
}
});