Skip to content

Commit 42df679

Browse files
trevnorristrevnorris
committed
node, async-wrap: remove MakeDomainCallback
C++ won't deoptimize like JS if specific conditional branches are sporadically met in the future. Combined with the amount of code duplication removal and simplified maintenance complexity, it makes more sense to merge MakeCallback and MakeDomainCallback. Additionally, type casting in V8 before verifying what that type is will cause V8 to abort in debug mode if that type isn't what was expected. Fix this by first checking the v8::Value before casting. PR-URL: nodejs/node-v0.x-archive#8110 Signed-off-by: Trevor Norris <[email protected]> Reviewed-by: Fedor Indutny <[email protected]> Reviewed-by: Alexis Campailla <[email protected]> Reviewed-by: Julien Gilli <[email protected]>
1 parent 2593c14 commit 42df679

4 files changed

Lines changed: 55 additions & 187 deletions

File tree

‎src/async-wrap-inl.h‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@
3131
#include "util-inl.h"
3232

3333
#include "v8.h"
34-
#include <assert.h>
3534

3635
namespace node {
3736

@@ -46,6 +45,7 @@ inline AsyncWrap::AsyncWrap(Environment* env,
4645
inline AsyncWrap::~AsyncWrap() {
4746
}
4847

48+
4949
inline uint32_t AsyncWrap::provider_type() const {
5050
return provider_type_;
5151
}
@@ -56,10 +56,8 @@ inline v8::Handle<v8::Value> AsyncWrap::MakeCallback(
5656
int argc,
5757
v8::Handle<v8::Value>* argv) {
5858
v8::Local<v8::Value> cb_v = object()->Get(symbol);
59-
v8::Local<v8::Function> cb = cb_v.As<v8::Function>();
60-
assert(cb->IsFunction());
61-
62-
return MakeCallback(cb, argc, argv);
59+
ASSERT(cb_v->IsFunction());
60+
return MakeCallback(cb_v.As<v8::Function>(), argc, argv);
6361
}
6462

6563

@@ -68,10 +66,8 @@ inline v8::Handle<v8::Value> AsyncWrap::MakeCallback(
6866
int argc,
6967
v8::Handle<v8::Value>* argv) {
7068
v8::Local<v8::Value> cb_v = object()->Get(index);
71-
v8::Local<v8::Function> cb = cb_v.As<v8::Function>();
72-
assert(cb->IsFunction());
73-
74-
return MakeCallback(cb, argc, argv);
69+
ASSERT(cb_v->IsFunction());
70+
return MakeCallback(cb_v.As<v8::Function>(), argc, argv);
7571
}
7672

7773
} // namespace node

‎src/async-wrap.cc‎

Lines changed: 20 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -37,30 +37,33 @@ using v8::Value;
3737

3838
namespace node {
3939

40-
Handle<Value> AsyncWrap::MakeDomainCallback(const Handle<Function> cb,
41-
int argc,
42-
Handle<Value>* argv) {
40+
Handle<Value> AsyncWrap::MakeCallback(const Handle<Function> cb,
41+
int argc,
42+
Handle<Value>* argv) {
4343
CHECK(env()->context() == env()->isolate()->GetCurrentContext());
4444

4545
Local<Object> context = object();
4646
Local<Object> process = env()->process_object();
47-
Local<Value> domain_v = context->Get(env()->domain_string());
4847
Local<Object> domain;
48+
bool has_domain = false;
49+
50+
if (env()->using_domains()) {
51+
Local<Value> domain_v = context->Get(env()->domain_string());
52+
has_domain = domain_v->IsObject();
53+
if (has_domain) {
54+
domain = domain_v.As<Object>();
55+
if (domain->Get(env()->disposed_string())->IsTrue())
56+
return Undefined(env()->isolate());
57+
}
58+
}
4959

5060
TryCatch try_catch;
5161
try_catch.SetVerbose(true);
5262

53-
bool has_domain = domain_v->IsObject();
5463
if (has_domain) {
55-
domain = domain_v.As<Object>();
56-
57-
if (domain->Get(env()->disposed_string())->IsTrue())
58-
return Undefined(env()->isolate());
59-
60-
Local<Function> enter =
61-
domain->Get(env()->enter_string()).As<Function>();
62-
if (enter->IsFunction()) {
63-
enter->Call(domain, 0, NULL);
64+
Local<Value> enter_v = domain->Get(env()->enter_string());
65+
if (enter_v->IsFunction()) {
66+
enter_v.As<Function>()->Call(domain, 0, NULL);
6467
if (try_catch.HasCaught())
6568
return Undefined(env()->isolate());
6669
}
@@ -73,10 +76,9 @@ Handle<Value> AsyncWrap::MakeDomainCallback(const Handle<Function> cb,
7376
}
7477

7578
if (has_domain) {
76-
Local<Function> exit =
77-
domain->Get(env()->exit_string()).As<Function>();
78-
if (exit->IsFunction()) {
79-
exit->Call(domain, 0, NULL);
79+
Local<Value> exit_v = domain->Get(env()->exit_string());
80+
if (exit_v->IsFunction()) {
81+
exit_v.As<Function>()->Call(domain, 0, NULL);
8082
if (try_catch.HasCaught())
8183
return Undefined(env()->isolate());
8284
}
@@ -111,54 +113,4 @@ Handle<Value> AsyncWrap::MakeDomainCallback(const Handle<Function> cb,
111113
return ret;
112114
}
113115

114-
115-
Handle<Value> AsyncWrap::MakeCallback(const Handle<Function> cb,
116-
int argc,
117-
Handle<Value>* argv) {
118-
if (env()->using_domains())
119-
return MakeDomainCallback(cb, argc, argv);
120-
121-
CHECK(env()->context() == env()->isolate()->GetCurrentContext());
122-
123-
Local<Object> context = object();
124-
Local<Object> process = env()->process_object();
125-
126-
TryCatch try_catch;
127-
try_catch.SetVerbose(true);
128-
129-
Local<Value> ret = cb->Call(context, argc, argv);
130-
131-
if (try_catch.HasCaught()) {
132-
return Undefined(env()->isolate());
133-
}
134-
135-
Environment::TickInfo* tick_info = env()->tick_info();
136-
137-
if (tick_info->in_tick()) {
138-
return ret;
139-
}
140-
141-
if (tick_info->length() == 0) {
142-
env()->isolate()->RunMicrotasks();
143-
}
144-
145-
if (tick_info->length() == 0) {
146-
tick_info->set_index(0);
147-
return ret;
148-
}
149-
150-
tick_info->set_in_tick(true);
151-
152-
env()->tick_callback_function()->Call(process, 0, NULL);
153-
154-
tick_info->set_in_tick(false);
155-
156-
if (try_catch.HasCaught()) {
157-
tick_info->set_last_threw(true);
158-
return Undefined(env()->isolate());
159-
}
160-
161-
return ret;
162-
}
163-
164116
} // namespace node

‎src/async-wrap.h‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -74,13 +74,6 @@ class AsyncWrap : public BaseObject {
7474
private:
7575
inline AsyncWrap();
7676

77-
// TODO(trevnorris): BURN IN FIRE! Remove this as soon as a suitable
78-
// replacement is committed.
79-
v8::Handle<v8::Value> MakeDomainCallback(
80-
const v8::Handle<v8::Function> cb,
81-
int argc,
82-
v8::Handle<v8::Value>* argv);
83-
8477
uint32_t provider_type_;
8578
};
8679

‎src/node.cc‎

Lines changed: 30 additions & 103 deletions
Original file line numberDiff line numberDiff line change
@@ -978,111 +978,53 @@ void SetupNextTick(const FunctionCallbackInfo<Value>& args) {
978978
}
979979

980980

981-
Handle<Value> MakeDomainCallback(Environment* env,
982-
Handle<Value> recv,
983-
const Handle<Function> callback,
984-
int argc,
985-
Handle<Value> argv[]) {
981+
Handle<Value> MakeCallback(Environment* env,
982+
Handle<Value> recv,
983+
const Handle<Function> callback,
984+
int argc,
985+
Handle<Value> argv[]) {
986986
// If you hit this assertion, you forgot to enter the v8::Context first.
987-
assert(env->context() == env->isolate()->GetCurrentContext());
987+
CHECK(env->context() == env->isolate()->GetCurrentContext());
988988

989989
Local<Object> process = env->process_object();
990990
Local<Object> object, domain;
991-
Local<Value> domain_v;
992-
993-
TryCatch try_catch;
994-
try_catch.SetVerbose(true);
995-
996991
bool has_domain = false;
997992

998-
if (!object.IsEmpty()) {
999-
domain_v = object->Get(env->domain_string());
993+
if (env->using_domains()) {
994+
CHECK(recv->IsObject());
995+
object = recv.As<Object>();
996+
Local<Value> domain_v = object->Get(env->domain_string());
1000997
has_domain = domain_v->IsObject();
1001998
if (has_domain) {
1002999
domain = domain_v.As<Object>();
1003-
1004-
if (domain->Get(env->disposed_string())->IsTrue()) {
1005-
// domain has been disposed of.
1000+
if (domain->Get(env->disposed_string())->IsTrue())
10061001
return Undefined(env->isolate());
1007-
}
1008-
1009-
Local<Function> enter = domain->Get(env->enter_string()).As<Function>();
1010-
if (enter->IsFunction()) {
1011-
enter->Call(domain, 0, NULL);
1012-
if (try_catch.HasCaught())
1013-
return Undefined(env->isolate());
1014-
}
10151002
}
10161003
}
10171004

1018-
Local<Value> ret = callback->Call(recv, argc, argv);
1019-
1020-
if (try_catch.HasCaught()) {
1021-
return Undefined(env->isolate());
1022-
}
1005+
TryCatch try_catch;
1006+
try_catch.SetVerbose(true);
10231007

10241008
if (has_domain) {
1025-
Local<Function> exit = domain->Get(env->exit_string()).As<Function>();
1026-
if (exit->IsFunction()) {
1027-
exit->Call(domain, 0, NULL);
1009+
Local<Value> enter_v = domain->Get(env->enter_string());
1010+
if (enter_v->IsFunction()) {
1011+
enter_v.As<Function>()->Call(domain, 0, NULL);
10281012
if (try_catch.HasCaught())
10291013
return Undefined(env->isolate());
10301014
}
10311015
}
10321016

1033-
Environment::TickInfo* tick_info = env->tick_info();
1034-
1035-
if (tick_info->last_threw() == 1) {
1036-
tick_info->set_last_threw(0);
1037-
return ret;
1038-
}
1039-
1040-
if (tick_info->in_tick()) {
1041-
return ret;
1042-
}
1043-
1044-
if (tick_info->length() == 0) {
1045-
env->isolate()->RunMicrotasks();
1046-
}
1047-
1048-
if (tick_info->length() == 0) {
1049-
tick_info->set_index(0);
1050-
return ret;
1051-
}
1052-
1053-
tick_info->set_in_tick(true);
1054-
1055-
env->tick_callback_function()->Call(process, 0, NULL);
1056-
1057-
tick_info->set_in_tick(false);
1017+
Local<Value> ret = callback->Call(recv, argc, argv);
10581018

1059-
if (try_catch.HasCaught()) {
1060-
tick_info->set_last_threw(true);
1061-
return Undefined(env->isolate());
1019+
if (has_domain) {
1020+
Local<Value> exit_v = domain->Get(env->exit_string());
1021+
if (exit_v->IsFunction()) {
1022+
exit_v.As<Function>()->Call(domain, 0, NULL);
1023+
if (try_catch.HasCaught())
1024+
return Undefined(env->isolate());
1025+
}
10621026
}
10631027

1064-
return ret;
1065-
}
1066-
1067-
1068-
Handle<Value> MakeCallback(Environment* env,
1069-
Handle<Value> recv,
1070-
const Handle<Function> callback,
1071-
int argc,
1072-
Handle<Value> argv[]) {
1073-
if (env->using_domains())
1074-
return MakeDomainCallback(env, recv, callback, argc, argv);
1075-
1076-
// If you hit this assertion, you forgot to enter the v8::Context first.
1077-
assert(env->context() == env->isolate()->GetCurrentContext());
1078-
1079-
Local<Object> process = env->process_object();
1080-
1081-
TryCatch try_catch;
1082-
try_catch.SetVerbose(true);
1083-
1084-
Local<Value> ret = callback->Call(recv, argc, argv);
1085-
10861028
if (try_catch.HasCaught()) {
10871029
return Undefined(env->isolate());
10881030
}
@@ -1124,10 +1066,9 @@ Handle<Value> MakeCallback(Environment* env,
11241066
uint32_t index,
11251067
int argc,
11261068
Handle<Value> argv[]) {
1127-
Local<Function> callback = recv->Get(index).As<Function>();
1128-
assert(callback->IsFunction());
1129-
1130-
return MakeCallback(env, recv.As<Value>(), callback, argc, argv);
1069+
Local<Value> cb_v = recv->Get(index);
1070+
CHECK(cb_v->IsFunction());
1071+
return MakeCallback(env, recv.As<Value>(), cb_v.As<Function>(), argc, argv);
11311072
}
11321073

11331074

@@ -1136,9 +1077,9 @@ Handle<Value> MakeCallback(Environment* env,
11361077
Handle<String> symbol,
11371078
int argc,
11381079
Handle<Value> argv[]) {
1139-
Local<Function> callback = recv->Get(symbol).As<Function>();
1140-
assert(callback->IsFunction());
1141-
return MakeCallback(env, recv.As<Value>(), callback, argc, argv);
1080+
Local<Value> cb_v = recv->Get(symbol);
1081+
CHECK(cb_v->IsFunction());
1082+
return MakeCallback(env, recv.As<Value>(), cb_v.As<Function>(), argc, argv);
11421083
}
11431084

11441085

@@ -1195,20 +1136,6 @@ Handle<Value> MakeCallback(Isolate* isolate,
11951136
}
11961137

11971138

1198-
Handle<Value> MakeDomainCallback(Handle<Object> recv,
1199-
Handle<Function> callback,
1200-
int argc,
1201-
Handle<Value> argv[]) {
1202-
Local<Context> context = recv->CreationContext();
1203-
Environment* env = Environment::GetCurrent(context);
1204-
Context::Scope context_scope(context);
1205-
EscapableHandleScope handle_scope(env->isolate());
1206-
return handle_scope.Escape(Local<Value>::New(
1207-
env->isolate(),
1208-
MakeDomainCallback(env, recv, callback, argc, argv)));
1209-
}
1210-
1211-
12121139
enum encoding ParseEncoding(Isolate* isolate,
12131140
Handle<Value> encoding_v,
12141141
enum encoding _default) {

0 commit comments

Comments
 (0)