Skip to content

Commit a1da024

Browse files
trevnorrispiscisaureus
trevnorris
authored and
piscisaureus
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 5d17b16 commit a1da024

File tree

4 files changed

+53
-183
lines changed

4 files changed

+53
-183
lines changed

‎src/async-wrap-inl.h‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,8 @@ inline v8::Handle<v8::Value> AsyncWrap::MakeCallback(
5151
int argc,
5252
v8::Handle<v8::Value>* argv) {
5353
v8::Local<v8::Value> cb_v = object()->Get(symbol);
54-
v8::Local<v8::Function> cb = cb_v.As<v8::Function>();
55-
CHECK(cb->IsFunction());
56-
57-
return MakeCallback(cb, argc, argv);
54+
CHECK(cb_v->IsFunction());
55+
return MakeCallback(cb_v.As<v8::Function>(), argc, argv);
5856
}
5957

6058

@@ -63,10 +61,8 @@ inline v8::Handle<v8::Value> AsyncWrap::MakeCallback(
6361
int argc,
6462
v8::Handle<v8::Value>* argv) {
6563
v8::Local<v8::Value> cb_v = object()->Get(index);
66-
v8::Local<v8::Function> cb = cb_v.As<v8::Function>();
67-
CHECK(cb->IsFunction());
68-
69-
return MakeCallback(cb, argc, argv);
64+
CHECK(cb_v->IsFunction());
65+
return MakeCallback(cb_v.As<v8::Function>(), argc, argv);
7066
}
7167

7268
} // 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, nullptr);
64+
Local<Value> enter_v = domain->Get(env()->enter_string());
65+
if (enter_v->IsFunction()) {
66+
enter_v.As<Function>()->Call(domain, 0, nullptr);
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, nullptr);
79+
Local<Value> exit_v = domain->Get(env()->exit_string());
80+
if (exit_v->IsFunction()) {
81+
exit_v.As<Function>()->Call(domain, 0, nullptr);
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, nullptr);
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: 29 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -986,111 +986,55 @@ void SetupNextTick(const FunctionCallbackInfo<Value>& args) {
986986
}
987987

988988

989-
Handle<Value> MakeDomainCallback(Environment* env,
990-
Handle<Value> recv,
991-
const Handle<Function> callback,
992-
int argc,
993-
Handle<Value> argv[]) {
989+
Handle<Value> MakeCallback(Environment* env,
990+
Handle<Value> recv,
991+
const Handle<Function> callback,
992+
int argc,
993+
Handle<Value> argv[]) {
994994
// If you hit this assertion, you forgot to enter the v8::Context first.
995995
CHECK_EQ(env->context(), env->isolate()->GetCurrentContext());
996996

997997
Local<Object> process = env->process_object();
998998
Local<Object> object, domain;
999-
Local<Value> domain_v;
1000-
1001-
TryCatch try_catch;
1002-
try_catch.SetVerbose(true);
1003-
1004999
bool has_domain = false;
10051000

1006-
if (!object.IsEmpty()) {
1007-
domain_v = object->Get(env->domain_string());
1001+
if (env->using_domains()) {
1002+
CHECK(recv->IsObject());
1003+
object = recv.As<Object>();
1004+
Local<Value> domain_v = object->Get(env->domain_string());
10081005
has_domain = domain_v->IsObject();
10091006
if (has_domain) {
10101007
domain = domain_v.As<Object>();
1011-
1012-
if (domain->Get(env->disposed_string())->IsTrue()) {
1013-
// domain has been disposed of.
1008+
if (domain->Get(env->disposed_string())->IsTrue())
10141009
return Undefined(env->isolate());
1015-
}
1016-
1017-
Local<Function> enter = domain->Get(env->enter_string()).As<Function>();
1018-
if (enter->IsFunction()) {
1019-
enter->Call(domain, 0, nullptr);
1020-
if (try_catch.HasCaught())
1021-
return Undefined(env->isolate());
1022-
}
10231010
}
10241011
}
10251012

1026-
Local<Value> ret = callback->Call(recv, argc, argv);
1027-
1028-
if (try_catch.HasCaught()) {
1029-
return Undefined(env->isolate());
1030-
}
1013+
TryCatch try_catch;
1014+
try_catch.SetVerbose(true);
10311015

10321016
if (has_domain) {
1033-
Local<Function> exit = domain->Get(env->exit_string()).As<Function>();
1034-
if (exit->IsFunction()) {
1035-
exit->Call(domain, 0, nullptr);
1017+
Local<Value> enter_v = domain->Get(env->enter_string());
1018+
if (enter_v->IsFunction()) {
1019+
enter_v.As<Function>()->Call(domain, 0, nullptr);
10361020
if (try_catch.HasCaught())
10371021
return Undefined(env->isolate());
10381022
}
10391023
}
10401024

1041-
Environment::TickInfo* tick_info = env->tick_info();
1042-
1043-
if (tick_info->last_threw() == 1) {
1044-
tick_info->set_last_threw(0);
1045-
return ret;
1046-
}
1047-
1048-
if (tick_info->in_tick()) {
1049-
return ret;
1050-
}
1051-
1052-
if (tick_info->length() == 0) {
1053-
env->isolate()->RunMicrotasks();
1054-
}
1025+
Local<Value> ret = callback->Call(recv, argc, argv);
10551026

1056-
if (tick_info->length() == 0) {
1057-
tick_info->set_index(0);
1058-
return ret;
1027+
if (has_domain) {
1028+
Local<Value> exit_v = domain->Get(env->exit_string());
1029+
if (exit_v->IsFunction()) {
1030+
exit_v.As<Function>()->Call(domain, 0, nullptr);
1031+
if (try_catch.HasCaught())
1032+
return Undefined(env->isolate());
1033+
}
10591034
}
1060-
1061-
tick_info->set_in_tick(true);
1062-
10631035
env->tick_callback_function()->Call(process, 0, nullptr);
1064-
1065-
tick_info->set_in_tick(false);
1066-
1067-
if (try_catch.HasCaught()) {
1068-
tick_info->set_last_threw(true);
1069-
return Undefined(env->isolate());
1070-
}
1071-
1072-
return ret;
1073-
}
1074-
1075-
1076-
Handle<Value> MakeCallback(Environment* env,
1077-
Handle<Value> recv,
1078-
const Handle<Function> callback,
1079-
int argc,
1080-
Handle<Value> argv[]) {
1081-
if (env->using_domains())
1082-
return MakeDomainCallback(env, recv, callback, argc, argv);
1083-
1084-
// If you hit this assertion, you forgot to enter the v8::Context first.
10851036
CHECK_EQ(env->context(), env->isolate()->GetCurrentContext());
10861037

1087-
Local<Object> process = env->process_object();
1088-
1089-
TryCatch try_catch;
1090-
try_catch.SetVerbose(true);
1091-
1092-
Local<Value> ret = callback->Call(recv, argc, argv);
1093-
10941038
if (try_catch.HasCaught()) {
10951039
return Undefined(env->isolate());
10961040
}
@@ -1132,10 +1076,9 @@ Handle<Value> MakeCallback(Environment* env,
11321076
uint32_t index,
11331077
int argc,
11341078
Handle<Value> argv[]) {
1135-
Local<Function> callback = recv->Get(index).As<Function>();
1136-
CHECK(callback->IsFunction());
1137-
1138-
return MakeCallback(env, recv.As<Value>(), callback, argc, argv);
1079+
Local<Value> cb_v = recv->Get(index);
1080+
CHECK(cb_v->IsFunction());
1081+
return MakeCallback(env, recv.As<Value>(), cb_v.As<Function>(), argc, argv);
11391082
}
11401083

11411084

@@ -1144,9 +1087,9 @@ Handle<Value> MakeCallback(Environment* env,
11441087
Handle<String> symbol,
11451088
int argc,
11461089
Handle<Value> argv[]) {
1147-
Local<Function> callback = recv->Get(symbol).As<Function>();
1148-
CHECK(callback->IsFunction());
1149-
return MakeCallback(env, recv.As<Value>(), callback, argc, argv);
1090+
Local<Value> cb_v = recv->Get(symbol);
1091+
CHECK(cb_v->IsFunction());
1092+
return MakeCallback(env, recv.As<Value>(), cb_v.As<Function>(), argc, argv);
11501093
}
11511094

11521095

@@ -1203,20 +1146,6 @@ Handle<Value> MakeCallback(Isolate* isolate,
12031146
}
12041147

12051148

1206-
Handle<Value> MakeDomainCallback(Handle<Object> recv,
1207-
Handle<Function> callback,
1208-
int argc,
1209-
Handle<Value> argv[]) {
1210-
Local<Context> context = recv->CreationContext();
1211-
Environment* env = Environment::GetCurrent(context);
1212-
Context::Scope context_scope(context);
1213-
EscapableHandleScope handle_scope(env->isolate());
1214-
return handle_scope.Escape(Local<Value>::New(
1215-
env->isolate(),
1216-
MakeDomainCallback(env, recv, callback, argc, argv)));
1217-
}
1218-
1219-
12201149
enum encoding ParseEncoding(Isolate* isolate,
12211150
Handle<Value> encoding_v,
12221151
enum encoding _default) {

0 commit comments

Comments
 (0)