From 80a55e9c83592b8c1552cdf08f0a5513c6d51ea5 Mon Sep 17 00:00:00 2001 From: isaacs Date: Fri, 6 Apr 2012 19:23:16 -0700 Subject: [PATCH 01/13] Report errors thrown from uncaughtException handers --- src/node.cc | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/node.cc b/src/node.cc index 09076432ba..3f1f911505 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1724,17 +1724,9 @@ static void OnFatalError(const char* location, const char* message) { exit(1); } -static int uncaught_exception_counter = 0; - void FatalException(TryCatch &try_catch) { HandleScope scope; - // Check if uncaught_exception_counter indicates a recursion - if (uncaught_exception_counter > 0) { - ReportException(try_catch, true); - exit(1); - } - if (listeners_symbol.IsEmpty()) { listeners_symbol = NODE_PSYMBOL("listeners"); uncaught_exception_symbol = NODE_PSYMBOL("uncaughtException"); @@ -1770,10 +1762,13 @@ void FatalException(TryCatch &try_catch) { Local error = try_catch.Exception(); Local event_argv[2] = { uncaught_exception_symbol_l, error }; - uncaught_exception_counter++; + TryCatch event_try_catch; emit->Call(process, 2, event_argv); - // Decrement so we know if the next exception is a recursion or not - uncaught_exception_counter--; + if (event_try_catch.HasCaught()) { + // the uncaught exception event threw, so we must exit. + ReportException(event_try_catch, true); + exit(1); + } } From 7407be896ef4cacaa96b383cb24b692c47debf62 Mon Sep 17 00:00:00 2001 From: isaacs Date: Fri, 13 Apr 2012 16:33:09 -0700 Subject: [PATCH 02/13] MakeCallback: Accept Function or Symbol argument --- src/node.cc | 32 ++++++++++++++++++++++++++------ src/node.h | 11 ++++++++++- 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/src/node.cc b/src/node.cc index 3f1f911505..4418eec998 100644 --- a/src/node.cc +++ b/src/node.cc @@ -972,19 +972,39 @@ Handle FromConstructorTemplate(Persistent& t, // // Maybe make this a method of a node::Handle super class // -void MakeCallback(Handle object, - const char* method, - int argc, - Handle argv[]) { +void +MakeCallback(const Handle object, + const char* method, + int argc, + Handle argv[]) { + HandleScope scope; + return scope.Close(MakeCallback(object, String::NewSymbol(method), argc, argv)); +} + +void +MakeCallback(const Handle object, + const Handle symbol, + int argc, + Handle argv[]) { HandleScope scope; - Local callback_v = object->Get(String::New(method)); + Local callback_v = object->Get(symbol); if (!callback_v->IsFunction()) { - fprintf(stderr, "method = %s", method); + String::Utf8Value method(symbol); + fprintf(stderr, "method = %s", *method); } assert(callback_v->IsFunction()); Local callback = Local::Cast(callback_v); +} + +void +MakeCallback(const Handle object, + const Handle callback, + int argc, + Handle argv[]) { + HandleScope scope; + // TODO Hook for long stack traces to be made here. TryCatch try_catch; diff --git a/src/node.h b/src/node.h index 788b65a964..7fb232b7c7 100644 --- a/src/node.h +++ b/src/node.h @@ -243,10 +243,19 @@ node_module_struct* get_builtin_module(const char *name); extern "C" node::node_module_struct modname ## _module; NODE_EXTERN void SetErrno(uv_err_t err); -NODE_EXTERN void MakeCallback(v8::Handle object, +NODE_EXTERN void MakeCallback(const v8::Handle object, const char* method, int argc, v8::Handle argv[]); +NODE_EXTERN void MakeCallback(const v8::Handle object, + const v8::Handle symbol, + int argc, + v8::Handle argv[]); + +NODE_EXTERN void MakeCallback(const v8::Handle object, + const v8::Handle callback, + int argc, + v8::Handle argv[]); } // namespace node #endif // SRC_NODE_H_ From ac1aaddc00103e7a2130d642fca90e0a5be18e16 Mon Sep 17 00:00:00 2001 From: isaacs Date: Fri, 13 Apr 2012 16:34:48 -0700 Subject: [PATCH 03/13] MakeCallback: Return the callback return value --- src/node.cc | 11 +++++++---- src/node.h | 27 +++++++++++++++------------ 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/src/node.cc b/src/node.cc index 4418eec998..26677eaeb6 100644 --- a/src/node.cc +++ b/src/node.cc @@ -972,7 +972,7 @@ Handle FromConstructorTemplate(Persistent& t, // // Maybe make this a method of a node::Handle super class // -void +Handle MakeCallback(const Handle object, const char* method, int argc, @@ -981,7 +981,7 @@ MakeCallback(const Handle object, return scope.Close(MakeCallback(object, String::NewSymbol(method), argc, argv)); } -void +Handle MakeCallback(const Handle object, const Handle symbol, int argc, @@ -996,9 +996,10 @@ MakeCallback(const Handle object, assert(callback_v->IsFunction()); Local callback = Local::Cast(callback_v); + return scope.Close(MakeCallback(object, callback, argc, argv)); } -void +Handle MakeCallback(const Handle object, const Handle callback, int argc, @@ -1009,11 +1010,13 @@ MakeCallback(const Handle object, TryCatch try_catch; - callback->Call(object, argc, argv); + Local ret = callback->Call(object, argc, argv); if (try_catch.HasCaught()) { FatalException(try_catch); } + + return scope.Close(ret); } diff --git a/src/node.h b/src/node.h index 7fb232b7c7..f7417c829f 100644 --- a/src/node.h +++ b/src/node.h @@ -243,19 +243,22 @@ node_module_struct* get_builtin_module(const char *name); extern "C" node::node_module_struct modname ## _module; NODE_EXTERN void SetErrno(uv_err_t err); -NODE_EXTERN void MakeCallback(const v8::Handle object, - const char* method, - int argc, - v8::Handle argv[]); +NODE_EXTERN v8::Handle +MakeCallback(const v8::Handle object, + const char* method, + int argc, + v8::Handle argv[]); -NODE_EXTERN void MakeCallback(const v8::Handle object, - const v8::Handle symbol, - int argc, - v8::Handle argv[]); +NODE_EXTERN v8::Handle +MakeCallback(const v8::Handle object, + const v8::Handle symbol, + int argc, + v8::Handle argv[]); -NODE_EXTERN void MakeCallback(const v8::Handle object, - const v8::Handle callback, - int argc, - v8::Handle argv[]); +NODE_EXTERN v8::Handle +MakeCallback(const v8::Handle object, + const v8::Handle callback, + int argc, + v8::Handle argv[]); } // namespace node #endif // SRC_NODE_H_ From 88f94fa28c4bc7f8a0cd080943a8169098bb33b2 Mon Sep 17 00:00:00 2001 From: isaacs Date: Mon, 16 Apr 2012 21:27:12 -0700 Subject: [PATCH 04/13] MakeCallback: abort() if not a function --- src/node.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/node.cc b/src/node.cc index 26677eaeb6..818c1a46bc 100644 --- a/src/node.cc +++ b/src/node.cc @@ -991,9 +991,13 @@ MakeCallback(const Handle object, Local callback_v = object->Get(symbol); if (!callback_v->IsFunction()) { String::Utf8Value method(symbol); - fprintf(stderr, "method = %s", *method); + // XXX: If the object has a domain attached, handle it there? + // At least, would be good to get *some* sort of indication + // of how we got here, even if it's not catchable. + fprintf(stderr, "Non-function in MakeCallback. method = %s\n", *method); + abort(); } - assert(callback_v->IsFunction()); + Local callback = Local::Cast(callback_v); return scope.Close(MakeCallback(object, callback, argc, argv)); From 91701c2db1055fcd7c79a5cacf7a0823d2bf1eb6 Mon Sep 17 00:00:00 2001 From: isaacs Date: Sun, 8 Apr 2012 00:48:10 -0700 Subject: [PATCH 05/13] MakeCallback: Use in node_file.cc --- src/node_file.cc | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index 60c094b243..099669c9d2 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -82,9 +82,6 @@ static void After(uv_fs_t *req) { FSReqWrap* req_wrap = (FSReqWrap*) req->data; assert(&req_wrap->req_ == req); - Local callback_v = req_wrap->object_->Get(oncomplete_sym); - assert(callback_v->IsFunction()); - Local callback = Local::Cast(callback_v); // there is always at least one argument. "error" int argc = 1; @@ -196,13 +193,7 @@ static void After(uv_fs_t *req) { } } - TryCatch try_catch; - - callback->Call(req_wrap->object_, argc, argv); - - if (try_catch.HasCaught()) { - FatalException(try_catch); - } + MakeCallback(req_wrap->object_, "oncomplete", argc, argv); uv_fs_req_cleanup(&req_wrap->req_); delete req_wrap; From 35c0cd219d0810c9e5324cf7d48cd6724b4e767b Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 11 Apr 2012 22:28:44 -0700 Subject: [PATCH 06/13] MakeCallback: Use in node_crypto --- src/node_crypto.cc | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index f508ba17d7..8646fea21c 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -861,16 +861,10 @@ int Connection::SelectSNIContextCallback_(SSL *s, int *ad, void* arg) { Local argv[1] = {*p->servername_}; Local callback = *p->sniCallback_; - TryCatch try_catch; - // Call it - Local ret = callback->Call(Context::GetCurrent()->Global(), - 1, - argv); - - if (try_catch.HasCaught()) { - FatalException(try_catch); - } + Local ret; + ret = Local::New(MakeCallback(Context::GetCurrent()->Global(), + callback, 1, argv)); // If ret is SecureContext if (secure_context_constructor->HasInstance(ret)) { @@ -4121,12 +4115,9 @@ EIO_PBKDF2After(uv_work_t* req) { argv[1] = Local::New(Undefined()); } - TryCatch try_catch; - - request->callback->Call(Context::GetCurrent()->Global(), 2, argv); - - if (try_catch.HasCaught()) - FatalException(try_catch); + MakeCallback(Context::GetCurrent()->Global(), + request->callback, + 2, argv); delete[] request->pass; delete[] request->salt; @@ -4314,11 +4305,9 @@ void RandomBytesAfter(uv_work_t* work_req) { Local argv[2]; RandomBytesCheck(req, argv); - TryCatch tc; - req->callback_->Call(Context::GetCurrent()->Global(), 2, argv); - - if (tc.HasCaught()) - FatalException(tc); + MakeCallback(Context::GetCurrent()->Global(), + req->callback_, + 2, argv); delete req; } From e1dd57058571c23c3a863ca00176cc0a99f6097e Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 11 Apr 2012 22:29:18 -0700 Subject: [PATCH 07/13] MakeCallback: Use in node_io_watcher --- src/node_io_watcher.cc | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/node_io_watcher.cc b/src/node_io_watcher.cc index 3d1ab51fa2..feb2b64011 100644 --- a/src/node_io_watcher.cc +++ b/src/node_io_watcher.cc @@ -65,17 +65,11 @@ void IOWatcher::Callback(EV_P_ ev_io *w, int revents) { Local callback = Local::Cast(callback_v); - TryCatch try_catch; - Local argv[2]; argv[0] = Local::New(revents & EV_READ ? True() : False()); argv[1] = Local::New(revents & EV_WRITE ? True() : False()); - callback->Call(io->handle_, 2, argv); - - if (try_catch.HasCaught()) { - FatalException(try_catch); - } + MakeCallback(io->handle_, callback, 2, argv); } From db45b2ca02dc23278a477625a8698ce136eb8807 Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 11 Apr 2012 22:29:36 -0700 Subject: [PATCH 08/13] MakeCallback: Use in node_signal_watcher --- src/node_signal_watcher.cc | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/node_signal_watcher.cc b/src/node_signal_watcher.cc index a0b57e0d20..8cfd272ece 100644 --- a/src/node_signal_watcher.cc +++ b/src/node_signal_watcher.cc @@ -62,13 +62,7 @@ void SignalWatcher::Callback(EV_P_ ev_signal *watcher, int revents) { Local callback = Local::Cast(callback_v); - TryCatch try_catch; - - callback->Call(w->handle_, 0, NULL); - - if (try_catch.HasCaught()) { - FatalException(try_catch); - } + MakeCallback(w->handle_, callback, 0, NULL); } Handle SignalWatcher::New(const Arguments& args) { From a26bee8fa16bcbdaafdee516288c6f59a43376f5 Mon Sep 17 00:00:00 2001 From: isaacs Date: Thu, 12 Apr 2012 16:03:47 -0700 Subject: [PATCH 09/13] MakeCallback: Consistent symbol usage --- src/cares_wrap.cc | 10 +++++----- src/fs_event_wrap.cc | 8 +++++++- src/node.cc | 9 ++++++--- src/node_buffer.cc | 4 ++-- src/node_crypto.cc | 18 +++++++++++++----- src/node_file.cc | 5 ++++- src/node_io_watcher.cc | 2 +- src/node_stat_watcher.cc | 12 ++++++++++-- src/node_zlib.cc | 4 ++-- src/pipe_wrap.cc | 13 +++++++++++-- src/process_wrap.cc | 6 +++++- src/stream_wrap.cc | 17 ++++++++++------- src/tcp_wrap.cc | 8 ++++++-- src/timer_wrap.cc | 5 ++++- src/udp_wrap.cc | 10 +++++++--- 15 files changed, 93 insertions(+), 38 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 5c5e030c3f..30d7238716 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -218,13 +218,13 @@ class QueryWrap { void CallOnComplete(Local answer) { HandleScope scope; Local argv[2] = { Integer::New(0), answer }; - MakeCallback(object_, "oncomplete", 2, argv); + MakeCallback(object_, oncomplete_sym, ARRAY_SIZE(argv), argv); } void CallOnComplete(Local answer, Local family) { HandleScope scope; Local argv[3] = { Integer::New(0), answer, family }; - MakeCallback(object_, "oncomplete", 3, argv); + MakeCallback(object_, oncomplete_sym, ARRAY_SIZE(argv), argv); } void ParseError(int status) { @@ -233,7 +233,7 @@ class QueryWrap { HandleScope scope; Local argv[1] = { Integer::New(-1) }; - MakeCallback(object_, "oncomplete", 1, argv); + MakeCallback(object_, oncomplete_sym, ARRAY_SIZE(argv), argv); } // Subclasses should implement the appropriate Parse method. @@ -679,7 +679,7 @@ void AfterGetAddrInfo(uv_getaddrinfo_t* req, int status, struct addrinfo* res) { uv_freeaddrinfo(res); // Make the callback into JavaScript - MakeCallback(req_wrap->object_, "oncomplete", 1, argv); + MakeCallback(req_wrap->object_, oncomplete_sym, ARRAY_SIZE(argv), argv); delete req_wrap; } @@ -755,7 +755,7 @@ static void Initialize(Handle target) { target->Set(String::NewSymbol("AF_INET6"), Integer::New(AF_INET6)); target->Set(String::NewSymbol("AF_UNSPEC"), Integer::New(AF_UNSPEC)); - oncomplete_sym = Persistent::New(String::NewSymbol("oncomplete")); + oncomplete_sym = NODE_PSYMBOL("oncomplete"); } diff --git a/src/fs_event_wrap.cc b/src/fs_event_wrap.cc index 2e66d29494..34bb9dfd6d 100644 --- a/src/fs_event_wrap.cc +++ b/src/fs_event_wrap.cc @@ -28,6 +28,8 @@ using namespace v8; namespace node { +static Persistent onchange_sym; + #define UNWRAP \ assert(!args.Holder().IsEmpty()); \ assert(args.Holder()->InternalFieldCount() > 0); \ @@ -165,7 +167,11 @@ void FSEventWrap::OnEvent(uv_fs_event_t* handle, const char* filename, filename ? (Local)String::New(filename) : Local::New(v8::Null()) }; - MakeCallback(wrap->object_, "onchange", 3, argv); + if (onchange_sym.IsEmpty()) { + onchange_sym = NODE_PSYMBOL("onchange"); + } + + MakeCallback(wrap->object_, onchange_sym, ARRAY_SIZE(argv), argv); } diff --git a/src/node.cc b/src/node.cc index 818c1a46bc..c5e15161b4 100644 --- a/src/node.cc +++ b/src/node.cc @@ -231,8 +231,7 @@ static void Tick(void) { if (tick_callback_sym.IsEmpty()) { // Lazily set the symbol - tick_callback_sym = - Persistent::New(String::NewSymbol("_tickCallback")); + tick_callback_sym = NODE_PSYMBOL("_tickCallback"); } Local cb_v = process->Get(tick_callback_sym); @@ -978,7 +977,11 @@ MakeCallback(const Handle object, int argc, Handle argv[]) { HandleScope scope; - return scope.Close(MakeCallback(object, String::NewSymbol(method), argc, argv)); + + Handle ret = + MakeCallback(object, String::NewSymbol(method), argc, argv); + + return scope.Close(ret); } Handle diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 7253662d60..35ddde531b 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -741,8 +741,8 @@ void Buffer::Initialize(Handle target) { assert(unbase64('\n') == -2); assert(unbase64('\r') == -2); - length_symbol = Persistent::New(String::NewSymbol("length")); - chars_written_sym = Persistent::New(String::NewSymbol("_charsWritten")); + length_symbol = NODE_PSYMBOL("length"); + chars_written_sym = NODE_PSYMBOL("_charsWritten"); Local t = FunctionTemplate::New(Buffer::New); constructor_template = Persistent::New(t); diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 8646fea21c..5f6594b8e5 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -82,6 +82,8 @@ static Persistent fingerprint_symbol; static Persistent name_symbol; static Persistent version_symbol; static Persistent ext_key_usage_symbol; +static Persistent onhandshakestart_sym; +static Persistent onhandshakedone_sym; static Persistent secure_context_constructor; @@ -864,7 +866,7 @@ int Connection::SelectSNIContextCallback_(SSL *s, int *ad, void* arg) { // Call it Local ret; ret = Local::New(MakeCallback(Context::GetCurrent()->Global(), - callback, 1, argv)); + callback, ARRAY_SIZE(argv), argv)); // If ret is SecureContext if (secure_context_constructor->HasInstance(ret)) { @@ -971,12 +973,18 @@ void Connection::SSLInfoCallback(const SSL *ssl, int where, int ret) { if (where & SSL_CB_HANDSHAKE_START) { HandleScope scope; Connection* c = static_cast(SSL_get_app_data(ssl)); - MakeCallback(c->handle_, "onhandshakestart", 0, NULL); + if (onhandshakestart_sym.IsEmpty()) { + onhandshakestart_sym = NODE_PSYMBOL("onhandshakestart"); + } + MakeCallback(c->handle_, onhandshakestart_sym, 0, NULL); } if (where & SSL_CB_HANDSHAKE_DONE) { HandleScope scope; Connection* c = static_cast(SSL_get_app_data(ssl)); - MakeCallback(c->handle_, "onhandshakedone", 0, NULL); + if (onhandshakedone_sym.IsEmpty()) { + onhandshakedone_sym = NODE_PSYMBOL("onhandshakedone"); + } + MakeCallback(c->handle_, onhandshakedone_sym, 0, NULL); } } @@ -4117,7 +4125,7 @@ EIO_PBKDF2After(uv_work_t* req) { MakeCallback(Context::GetCurrent()->Global(), request->callback, - 2, argv); + ARRAY_SIZE(argv), argv); delete[] request->pass; delete[] request->salt; @@ -4307,7 +4315,7 @@ void RandomBytesAfter(uv_work_t* work_req) { MakeCallback(Context::GetCurrent()->Global(), req->callback_, - 2, argv); + ARRAY_SIZE(argv), argv); delete req; } diff --git a/src/node_file.cc b/src/node_file.cc index 099669c9d2..f4b53a1b2f 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -193,7 +193,10 @@ static void After(uv_fs_t *req) { } } - MakeCallback(req_wrap->object_, "oncomplete", argc, argv); + if (oncomplete_sym.IsEmpty()) { + oncomplete_sym = NODE_PSYMBOL("oncomplete"); + } + MakeCallback(req_wrap->object_, oncomplete_sym, argc, argv); uv_fs_req_cleanup(&req_wrap->req_); delete req_wrap; diff --git a/src/node_io_watcher.cc b/src/node_io_watcher.cc index feb2b64011..ad9a7b63fc 100644 --- a/src/node_io_watcher.cc +++ b/src/node_io_watcher.cc @@ -69,7 +69,7 @@ void IOWatcher::Callback(EV_P_ ev_io *w, int revents) { argv[0] = Local::New(revents & EV_READ ? True() : False()); argv[1] = Local::New(revents & EV_WRITE ? True() : False()); - MakeCallback(io->handle_, callback, 2, argv); + MakeCallback(io->handle_, callback, ARRAY_SIZE(argv), argv); } diff --git a/src/node_stat_watcher.cc b/src/node_stat_watcher.cc index e38d581113..efe3939e9b 100644 --- a/src/node_stat_watcher.cc +++ b/src/node_stat_watcher.cc @@ -30,6 +30,8 @@ namespace node { using namespace v8; Persistent StatWatcher::constructor_template; +static Persistent onchange_sym; +static Persistent onstop_sym; void StatWatcher::Initialize(Handle target) { HandleScope scope; @@ -54,7 +56,10 @@ void StatWatcher::Callback(EV_P_ ev_stat *watcher, int revents) { Local argv[2]; argv[0] = BuildStatsObject(&watcher->attr); argv[1] = BuildStatsObject(&watcher->prev); - MakeCallback(handler->handle_, "onchange", 2, argv); + if (onchange_sym.IsEmpty()) { + onchange_sym = NODE_PSYMBOL("onchange"); + } + MakeCallback(handler->handle_, onchange_sym, ARRAY_SIZE(argv), argv); } @@ -106,7 +111,10 @@ Handle StatWatcher::Start(const Arguments& args) { Handle StatWatcher::Stop(const Arguments& args) { HandleScope scope; StatWatcher *handler = ObjectWrap::Unwrap(args.Holder()); - MakeCallback(handler->handle_, "onstop", 0, NULL); + if (onstop_sym.IsEmpty()) { + onstop_sym = NODE_PSYMBOL("onstop"); + } + MakeCallback(handler->handle_, onstop_sym, 0, NULL); handler->Stop(); return Undefined(); } diff --git a/src/node_zlib.cc b/src/node_zlib.cc index e34c5ce1a0..9a53df235d 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -211,7 +211,7 @@ class ZCtx : public ObjectWrap { assert(ctx->handle_->Get(callback_sym)->IsFunction() && "Invalid callback"); Local args[2] = { avail_in, avail_out }; - MakeCallback(ctx->handle_, "callback", 2, args); + MakeCallback(ctx->handle_, callback_sym, ARRAY_SIZE(args), args); ctx->Unref(); } @@ -229,7 +229,7 @@ class ZCtx : public ObjectWrap { HandleScope scope; Local args[2] = { String::New(msg), Local::New(Number::New(ctx->err_)) }; - MakeCallback(ctx->handle_, "onerror", ARRAY_SIZE(args), args); + MakeCallback(ctx->handle_, onerror_sym, ARRAY_SIZE(args), args); // no hope of rescue. ctx->Unref(); diff --git a/src/pipe_wrap.cc b/src/pipe_wrap.cc index 45902d4a88..8065461c69 100644 --- a/src/pipe_wrap.cc +++ b/src/pipe_wrap.cc @@ -57,6 +57,9 @@ using v8::Boolean; Persistent pipeConstructor; +static Persistent onconnection_sym; +static Persistent oncomplete_sym; + // TODO share with TCPWrap? typedef class ReqWrap ConnectWrap; @@ -215,7 +218,10 @@ void PipeWrap::OnConnection(uv_stream_t* handle, int status) { // Successful accept. Call the onconnection callback in JavaScript land. Local argv[1] = { client_obj }; - MakeCallback(wrap->object_, "onconnection", 1, argv); + if (onconnection_sym.IsEmpty()) { + onconnection_sym = NODE_PSYMBOL("onconnection"); + } + MakeCallback(wrap->object_, onconnection_sym, ARRAY_SIZE(argv), argv); } // TODO Maybe share this with TCPWrap? @@ -247,7 +253,10 @@ void PipeWrap::AfterConnect(uv_connect_t* req, int status) { Local::New(Boolean::New(writable)) }; - MakeCallback(req_wrap->object_, "oncomplete", 5, argv); + if (oncomplete_sym.IsEmpty()) { + oncomplete_sym = NODE_PSYMBOL("oncomplete"); + } + MakeCallback(req_wrap->object_, oncomplete_sym, ARRAY_SIZE(argv), argv); delete req_wrap; } diff --git a/src/process_wrap.cc b/src/process_wrap.cc index 159473b42f..aee9a01501 100644 --- a/src/process_wrap.cc +++ b/src/process_wrap.cc @@ -54,6 +54,7 @@ using v8::Context; using v8::Arguments; using v8::Integer; +static Persistent onexit_sym; class ProcessWrap : public HandleWrap { public: @@ -223,7 +224,10 @@ class ProcessWrap : public HandleWrap { String::New(signo_string(term_signal)) }; - MakeCallback(wrap->object_, "onexit", 2, argv); + if (onexit_sym.IsEmpty()) { + onexit_sym = NODE_PSYMBOL("onexit"); + } + MakeCallback(wrap->object_, onexit_sym, ARRAY_SIZE(argv), argv); } uv_process_t process_; diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index 234aa3dc2f..9fcb5b1b95 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -69,6 +69,8 @@ typedef class ReqWrap WriteWrap; static Persistent buffer_sym; static Persistent write_queue_size_sym; +static Persistent onread_sym; +static Persistent oncomplete_sym; static SlabAllocator slab_allocator(SLAB_SIZE); static bool initialized; @@ -81,9 +83,10 @@ void StreamWrap::Initialize(Handle target) { HandleWrap::Initialize(target); - buffer_sym = Persistent::New(String::NewSymbol("buffer")); - write_queue_size_sym = - Persistent::New(String::NewSymbol("writeQueueSize")); + buffer_sym = NODE_PSYMBOL("buffer"); + write_queue_size_sym = NODE_PSYMBOL("writeQueueSize"); + onread_sym = NODE_PSYMBOL("onread"); + oncomplete_sym = NODE_PSYMBOL("oncomplete"); } @@ -170,7 +173,7 @@ void StreamWrap::OnReadCommon(uv_stream_t* handle, ssize_t nread, } SetErrno(uv_last_error(uv_default_loop())); - MakeCallback(wrap->object_, "onread", 0, NULL); + MakeCallback(wrap->object_, onread_sym, 0, NULL); return; } @@ -208,7 +211,7 @@ void StreamWrap::OnReadCommon(uv_stream_t* handle, ssize_t nread, argc++; } - MakeCallback(wrap->object_, "onread", argc, argv); + MakeCallback(wrap->object_, onread_sym, argc, argv); } @@ -313,7 +316,7 @@ void StreamWrap::AfterWrite(uv_write_t* req, int status) { req_wrap->object_->GetHiddenValue(buffer_sym), }; - MakeCallback(req_wrap->object_, "oncomplete", 4, argv); + MakeCallback(req_wrap->object_, oncomplete_sym, ARRAY_SIZE(argv), argv); delete req_wrap; } @@ -360,7 +363,7 @@ void StreamWrap::AfterShutdown(uv_shutdown_t* req, int status) { Local::New(req_wrap->object_) }; - MakeCallback(req_wrap->object_, "oncomplete", 3, argv); + MakeCallback(req_wrap->object_, oncomplete_sym, ARRAY_SIZE(argv), argv); delete req_wrap; } diff --git a/src/tcp_wrap.cc b/src/tcp_wrap.cc index e571bc5124..6aee0c9093 100644 --- a/src/tcp_wrap.cc +++ b/src/tcp_wrap.cc @@ -77,6 +77,8 @@ static Persistent tcpConstructor; static Persistent family_symbol; static Persistent address_symbol; static Persistent port_symbol; +static Persistent oncomplete_sym; +static Persistent onconnection_sym; typedef class ReqWrap ConnectWrap; @@ -131,6 +133,8 @@ void TCPWrap::Initialize(Handle target) { family_symbol = NODE_PSYMBOL("family"); address_symbol = NODE_PSYMBOL("address"); port_symbol = NODE_PSYMBOL("port"); + onconnection_sym = NODE_PSYMBOL("onconnection"); + oncomplete_sym = NODE_PSYMBOL("oncomplete"); target->Set(String::NewSymbol("TCP"), tcpConstructor); } @@ -380,7 +384,7 @@ void TCPWrap::OnConnection(uv_stream_t* handle, int status) { argv[0] = Local::New(Null()); } - MakeCallback(wrap->object_, "onconnection", 1, argv); + MakeCallback(wrap->object_, onconnection_sym, ARRAY_SIZE(argv), argv); } @@ -406,7 +410,7 @@ void TCPWrap::AfterConnect(uv_connect_t* req, int status) { Local::New(v8::True()) }; - MakeCallback(req_wrap->object_, "oncomplete", 5, argv); + MakeCallback(req_wrap->object_, oncomplete_sym, ARRAY_SIZE(argv), argv); delete req_wrap; } diff --git a/src/timer_wrap.cc b/src/timer_wrap.cc index 273f8265f4..89ebf46bb0 100644 --- a/src/timer_wrap.cc +++ b/src/timer_wrap.cc @@ -50,6 +50,7 @@ using v8::Context; using v8::Arguments; using v8::Integer; +static Persistent ontimeout_sym; class TimerWrap : public HandleWrap { public: @@ -70,6 +71,8 @@ class TimerWrap : public HandleWrap { NODE_SET_PROTOTYPE_METHOD(constructor, "getRepeat", GetRepeat); NODE_SET_PROTOTYPE_METHOD(constructor, "again", Again); + ontimeout_sym = NODE_PSYMBOL("ontimeout"); + target->Set(String::NewSymbol("Timer"), constructor->GetFunction()); } @@ -200,7 +203,7 @@ class TimerWrap : public HandleWrap { wrap->StateChange(); Local argv[1] = { Integer::New(status) }; - MakeCallback(wrap->object_, "ontimeout", 1, argv); + MakeCallback(wrap->object_, ontimeout_sym, ARRAY_SIZE(argv), argv); } uv_timer_t handle_; diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc index 5ed91253ee..50f5d64c08 100644 --- a/src/udp_wrap.cc +++ b/src/udp_wrap.cc @@ -68,6 +68,8 @@ Local AddressToJS(const sockaddr* addr); static Persistent address_symbol; static Persistent port_symbol; static Persistent buffer_sym; +static Persistent oncomplete_sym; +static Persistent onmessage_sym; static SlabAllocator slab_allocator(SLAB_SIZE); @@ -130,6 +132,8 @@ void UDPWrap::Initialize(Handle target) { buffer_sym = NODE_PSYMBOL("buffer"); port_symbol = NODE_PSYMBOL("port"); address_symbol = NODE_PSYMBOL("address"); + oncomplete_sym = NODE_PSYMBOL("oncomplete"); + onmessage_sym = NODE_PSYMBOL("onmessage"); Local t = FunctionTemplate::New(New); t->InstanceTemplate()->SetInternalFieldCount(1); @@ -394,7 +398,7 @@ void UDPWrap::OnSend(uv_udp_send_t* req, int status) { req_wrap->object_->GetHiddenValue(buffer_sym), }; - MakeCallback(req_wrap->object_, "oncomplete", 4, argv); + MakeCallback(req_wrap->object_, oncomplete_sym, ARRAY_SIZE(argv), argv); delete req_wrap; } @@ -422,7 +426,7 @@ void UDPWrap::OnRecv(uv_udp_t* handle, if (nread < 0) { Local argv[] = { Local::New(wrap->object_) }; SetErrno(uv_last_error(uv_default_loop())); - MakeCallback(wrap->object_, "onmessage", ARRAY_SIZE(argv), argv); + MakeCallback(wrap->object_, onmessage_sym, ARRAY_SIZE(argv), argv); return; } @@ -433,7 +437,7 @@ void UDPWrap::OnRecv(uv_udp_t* handle, Integer::NewFromUnsigned(nread), AddressToJS(addr) }; - MakeCallback(wrap->object_, "onmessage", ARRAY_SIZE(argv), argv); + MakeCallback(wrap->object_, onmessage_sym, ARRAY_SIZE(argv), argv); } From 963459d736d6594de641aff4d8767da113359457 Mon Sep 17 00:00:00 2001 From: isaacs Date: Fri, 6 Apr 2012 16:26:18 -0700 Subject: [PATCH 10/13] Domain feature This is a squashed commit of the main work done on the domains-wip branch. The original commit messages are preserved for posterity: * Implicitly add EventEmitters to active domain * Implicitly add timers to active domain * domain: add members, remove ctor cb * Don't hijack bound callbacks for Domain error events * Add dispose method * Add domain.remove(ee) method * A test of multiple domains in process at once * Put the active domain on the process object * Only intercept error arg if explicitly requested * Typo * Don't auto-add new domains to the current domain While an automatic parent/child relationship is sort of neat, and leads to some nice error-bubbling characteristics, it also results in keeping a reference to every EE and timer created, unless domains are explicitly disposed of. * Explicitly adding one domain to another is still fine, of course. * Don't allow circular domain->domain memberships * Disposing of a domain removes it from its parent * Domain disposal turns functions into no-ops * More documentation of domains * More thorough dispose() semantics * An example using domains in an HTTP server * Don't handle errors on a disposed domain * Need to push, even if the same domain is entered multiple times * Array.push is too slow for the EE Ctor * lint domain * domain: docs * Also call abort and destroySoon to clean up event emitters * domain: Wrap destroy methods in a try/catch * Attach tick callbacks to active domain * domain: Only implicitly bind timers, not explicitly * domain: Don't fire timers when disposed. * domain: Simplify naming so that MakeCallback works on Timers * Add setInterval and nextTick to domain test * domain: Make stack private --- doc/api/_toc.markdown | 1 + doc/api/all.markdown | 1 + doc/api/domain.markdown | 181 +++++++++++++++++++ lib/domain.js | 233 +++++++++++++++++++++++++ lib/events.js | 31 +++- lib/timers.js | 9 + node.gyp | 1 + src/node.js | 12 +- test/simple/test-domain-http-server.js | 115 ++++++++++++ test/simple/test-domain-multi.js | 100 +++++++++++ test/simple/test-domain.js | 198 +++++++++++++++++++++ 11 files changed, 879 insertions(+), 3 deletions(-) create mode 100644 doc/api/domain.markdown create mode 100644 lib/domain.js create mode 100644 test/simple/test-domain-http-server.js create mode 100644 test/simple/test-domain-multi.js create mode 100644 test/simple/test-domain.js diff --git a/doc/api/_toc.markdown b/doc/api/_toc.markdown index 73e6b9bd58..0e90fe6c76 100644 --- a/doc/api/_toc.markdown +++ b/doc/api/_toc.markdown @@ -8,6 +8,7 @@ * [Process](process.html) * [Utilities](util.html) * [Events](events.html) +* [Domain](domain.html) * [Buffer](buffer.html) * [Stream](stream.html) * [Crypto](crypto.html) diff --git a/doc/api/all.markdown b/doc/api/all.markdown index 7b7f736956..c62526713e 100644 --- a/doc/api/all.markdown +++ b/doc/api/all.markdown @@ -8,6 +8,7 @@ @include process @include util @include events +@include domain @include buffer @include stream @include crypto diff --git a/doc/api/domain.markdown b/doc/api/domain.markdown new file mode 100644 index 0000000000..7e546fd537 --- /dev/null +++ b/doc/api/domain.markdown @@ -0,0 +1,181 @@ +# Domain + + Stability: 1 - Experimental + +Domains provide a way to handle multiple different IO operations as a +single group. If any of the event emitters or callbacks registered to a +domain emit an `error` event, or throw an error, then the domain object +will be notified, rather than losing the context of the error in the +`process.on('uncaughtException')` handler, or causing the program to +exit with an error code. + +This feature is new in Node version 0.8. It is a first pass, and is +expected to change significantly in future versions. Please use it and +provide feedback. + +Due to their experimental nature, the Domains features are disabled unless +the `domain` module is loaded at least once. No domains are created or +registered by default. This is by design, to prevent adverse effects on +current programs. It is expected to be enabled by default in future +Node.js versions. + +## Additions to Error objects + + + +Any time an Error object is routed through a domain, a few extra fields +are added to it. + +* `error.domain` The domain that first handled the error. +* `error.domain_emitter` The event emitter that emitted an 'error' event + with the error object. +* `error.domain_bound` The callback function which was bound to the + domain, and passed an error as its first argument. +* `error.domain_thrown` A boolean indicating whether the error was + thrown, emitted, or passed to a bound callback function. + +## Implicit Binding + + + +If domains are in use, then all new EventEmitter objects (including +Stream objects, requests, responses, etc.) will be implicitly bound to +the active domain at the time of their creation. + +Additionally, callbacks passed to lowlevel event loop requests (such as +to fs.open, or other callback-taking methods) will automatically be +bound to the active domain. If they throw, then the domain will catch +the error. + +In order to prevent excessive memory usage, Domain objects themselves +are not implicitly added as children of the active domain. If they +were, then it would be too easy to prevent request and response objects +from being properly garbage collected. + +If you *want* to nest Domain objects as children of a parent Domain, +then you must explicitly add them, and then dispose of them later. + +Implicit binding routes thrown errors and `'error'` events to the +Domain's `error` event, but does not register the EventEmitter on the +Domain, so `domain.dispose()` will not shut down the EventEmitter. +Implicit binding only takes care of thrown errors and `'error'` events. + +## domain.create() + +* return: {Domain} + +Returns a new Domain object. + +## Class: Domain + +The Domain class encapsulates the functionality of routing errors and +uncaught exceptions to the active Domain object. + +Domain is a child class of EventEmitter. To handle the errors that it +catches, listen to its `error` event. + +### domain.members + +* {Array} + +An array of timers and event emitters that have been explicitly added +to the domain. + +### domain.add(emitter) + +* `emitter` {EventEmitter | Timer} emitter or timer to be added to the domain + +Explicitly adds an emitter to the domain. If any event handlers called by +the emitter throw an error, or if the emitter emits an `error` event, it +will be routed to the domain's `error` event, just like with implicit +binding. + +This also works with timers that are returned from `setInterval` and +`setTimeout`. If their callback function throws, it will be caught by +the domain 'error' handler. + +If the Timer or EventEmitter was already bound to a domain, it is removed +from that one, and bound to this one instead. + +### domain.remove(emitter) + +* `emitter` {EventEmitter | Timer} emitter or timer to be removed from the domain + +The opposite of `domain.add(emitter)`. Removes domain handling from the +specified emitter. + +### domain.bind(cb) + +* `cb` {Function} The callback function +* return: {Function} The bound function + +The returned function will be a wrapper around the supplied callback +function. When the returned function is called, any errors that are +thrown will be routed to the domain's `error` event. + +#### Example + + var d = domain.create(); + + function readSomeFile(filename, cb) { + fs.readFile(filename, d.bind(function(er, data) { + // if this throws, it will also be passed to the domain + return cb(er, JSON.parse(data)); + })); + } + + d.on('error', function(er) { + // an error occurred somewhere. + // if we throw it now, it will crash the program + // with the normal line number and stack message. + }); + +### domain.intercept(cb) + +* `cb` {Function} The callback function +* return: {Function} The intercepted function + +This method is almost identical to `domain.bind(cb)`. However, in +addition to catching thrown errors, it will also intercept `Error` +objects sent as the first argument to the function. + +In this way, the common `if (er) return cb(er);` pattern can be replaced +with a single error handler in a single place. + +#### Example + + var d = domain.create(); + + function readSomeFile(filename, cb) { + fs.readFile(filename, d.intercept(function(er, data) { + // if this throws, it will also be passed to the domain + // additionally, we know that 'er' will always be null, + // so the error-handling logic can be moved to the 'error' + // event on the domain instead of being repeated throughout + // the program. + return cb(er, JSON.parse(data)); + })); + } + + d.on('error', function(er) { + // an error occurred somewhere. + // if we throw it now, it will crash the program + // with the normal line number and stack message. + }); + +### domain.dispose() + +The dispose method destroys a domain, and makes a best effort attempt to +clean up any and all IO that is associated with the domain. Streams are +aborted, ended, closed, and/or destroyed. Timers are cleared. +Explicitly bound callbacks are no longer called. Any error events that +are raised as a result of this are ignored. + +The intention of calling `dispose` is generally to prevent cascading +errors when a critical part of the Domain context is found to be in an +error state. + +Note that IO might still be performed. However, to the highest degree +possible, once a domain is disposed, further errors from the emitters in +that set will be ignored. So, even if some remaining actions are still +in flight, Node.js will not communicate further about them. diff --git a/lib/domain.js b/lib/domain.js new file mode 100644 index 0000000000..d7a71ed1f3 --- /dev/null +++ b/lib/domain.js @@ -0,0 +1,233 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +var util = require('util'); +var events = require('events'); +var EventEmitter = events.EventEmitter; +var inherits = util.inherits; + +// methods that are called when trying to shut down expliclitly bound EEs +var endMethods = [ 'end', 'abort', 'destroy', 'destroySoon' ]; + +// communicate with events module, but don't require that +// module to have to load this one, since this module has +// a few side effects. +events.usingDomains = true; + +exports.Domain = Domain; + +exports.create = exports.createDomain = function(cb) { + return new Domain(cb); +}; + +// it's possible to enter one domain while already inside +// another one. the stack is each entered domain. +var stack = []; +// the active domain is always the one that we're currently in. +exports.active = null; + + +// loading this file the first time sets up the global +// uncaughtException handler. +process.on('uncaughtException', uncaughtHandler); + +function uncaughtHandler(er) { + // if there's an active domain, then handle this there. + // Note that if this error emission throws, then it'll just crash. + if (exports.active && !exports.active._disposed) { + decorate(er, { + domain: exports.active, + domain_thrown: true + }); + exports.active.emit('error', er); + } else if (process.listeners('uncaughtException').length === 1) { + // if there are other handlers, then they'll take care of it. + // but if not, then we need to crash now. + throw er; + } +} + +inherits(Domain, EventEmitter); + +function Domain() { + EventEmitter.apply(this); + + this.members = []; +} + +Domain.prototype.enter = function() { + if (this._disposed) return; + + // note that this might be a no-op, but we still need + // to push it onto the stack so that we can pop it later. + exports.active = process.domain = this; + stack.push(this); +}; + +Domain.prototype.exit = function() { + if (this._disposed) return; + + // exit all domains until this one. + var d; + do { + d = stack.pop(); + } while (d && d !== this); + + exports.active = stack[stack.length - 1]; + process.domain = exports.active; +}; + +// note: this works for timers as well. +Domain.prototype.add = function(ee) { + // disposed domains can't be used for new things. + if (this._disposed) return; + + // already added to this domain. + if (ee.domain === this) return; + + // has a domain already - remove it first. + if (ee.domain) { + ee.domain.remove(ee); + } + + // check for circular Domain->Domain links. + // This causes bad insanity! + // + // For example: + // var d = domain.create(); + // var e = domain.create(); + // d.add(e); + // e.add(d); + // e.emit('error', er); // RangeError, stack overflow! + if (this.domain && (ee instanceof Domain)) { + for (var d = this.domain; d; d = d.domain) { + if (ee === d) return; + } + } + + ee.domain = this; + this.members.push(ee); +}; + +Domain.prototype.remove = function(ee) { + ee.domain = null; + var index = this.members.indexOf(ee); + if (index !== -1) { + this.members.splice(index, 1); + } +}; + +Domain.prototype.run = function(fn) { + this.bind(fn)(); +}; + +Domain.prototype.intercept = function(cb) { + return this.bind(cb, true); +}; + +Domain.prototype.bind = function(cb, interceptError) { + // if cb throws, catch it here. + var self = this; + var b = function() { + // disposing turns functions into no-ops + if (self._disposed) return; + + if (this instanceof Domain) { + return cb.apply(this, arguments); + } + + // only intercept first-arg errors if explicitly requested. + if (interceptError && arguments[0] && + (arguments[0] instanceof Error)) { + var er = arguments[0]; + decorate(er, { + domain_bound: cb, + domain_thrown: false, + domain: self + }); + self.emit('error', er); + return; + } + + self.enter(); + var ret = cb.apply(this, arguments); + self.exit(); + return ret; + }; + b.domain = this; + return b; +}; + +Domain.prototype.dispose = function() { + if (this._disposed) return; + + this.emit('dispose'); + + // remove error handlers. + this.removeAllListeners(); + this.on('error', function() {}); + + // try to kill all the members. + // XXX There should be more consistent ways + // to shut down things! + this.members.forEach(function(m) { + // if it's a timeout or interval, cancel it. + clearTimeout(m); + + // drop all event listeners. + if (m instanceof EventEmitter) { + m.removeAllListeners(); + // swallow errors + m.on('error', function() {}); + } + + // Be careful! + // By definition, we're likely in error-ridden territory here, + // so it's quite possible that calling some of these methods + // might cause additional exceptions to be thrown. + endMethods.forEach(function(method) { + if (typeof m[method] === 'function') { + try { + m[method](); + } catch (er) {} + } + }); + + }); + + // remove from parent domain, if there is one. + if (this.domain) this.domain.remove(this); + + // kill the references so that they can be properly gc'ed. + this.members.length = 0; + + // finally, mark this domain as 'no longer relevant' + // so that it can't be entered or activated. + this._disposed = true; +}; + + +function decorate(er, props) { + Object.keys(props).forEach(function(k, _, __) { + if (er.hasOwnProperty(k)) return; + er[k] = props[k]; + }); +} diff --git a/lib/events.js b/lib/events.js index 05255ac3d3..c4ab9d80a0 100644 --- a/lib/events.js +++ b/lib/events.js @@ -21,7 +21,15 @@ var isArray = Array.isArray; -function EventEmitter() { } +function EventEmitter() { + if (exports.usingDomains) { + // if there is an active domain, then attach to it. + var domain = require('domain'); + if (domain.active && !(this instanceof domain.Domain)) { + this.domain = domain.active; + } + } +} exports.EventEmitter = EventEmitter; // By default EventEmitters will print a warning if more than @@ -44,6 +52,15 @@ EventEmitter.prototype.emit = function() { if (!this._events || !this._events.error || (isArray(this._events.error) && !this._events.error.length)) { + if (this.domain) { + var er = arguments[1]; + er.domain_emitter = this; + er.domain = this.domain; + er.domain_thrown = false; + this.domain.emit('error', er); + return false; + } + if (arguments[1] instanceof Error) { throw arguments[1]; // Unhandled 'error' event } else { @@ -58,6 +75,9 @@ EventEmitter.prototype.emit = function() { if (!handler) return false; if (typeof handler == 'function') { + if (this.domain) { + this.domain.enter(); + } switch (arguments.length) { // fast cases case 1: @@ -76,9 +96,15 @@ EventEmitter.prototype.emit = function() { for (var i = 1; i < l; i++) args[i - 1] = arguments[i]; handler.apply(this, args); } + if (this.domain) { + this.domain.exit(); + } return true; } else if (isArray(handler)) { + if (this.domain) { + this.domain.enter(); + } var l = arguments.length; var args = new Array(l - 1); for (var i = 1; i < l; i++) args[i - 1] = arguments[i]; @@ -87,6 +113,9 @@ EventEmitter.prototype.emit = function() { for (var i = 0, l = listeners.length; i < l; i++) { listeners[i].apply(this, args); } + if (this.domain) { + this.domain.exit(); + } return true; } else { diff --git a/lib/timers.js b/lib/timers.js index 97e5830e6e..9e2336d058 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -93,12 +93,17 @@ function insert(item, msecs) { // hack should be removed. // // https://github.com/joyent/node/issues/2631 + if (first.domain) { + if (first.domain._disposed) continue; + first.domain.enter(); + } try { first._onTimeout(); } catch (e) { if (!process.listeners('uncaughtException').length) throw e; process.emit('uncaughtException', e); } + if (first.domain) first.domain.exit(); } } @@ -192,6 +197,8 @@ exports.setTimeout = function(callback, after) { } } + if (process.domain) timer.domain = process.domain; + exports.active(timer); return timer; @@ -213,6 +220,8 @@ exports.clearTimeout = function(timer) { exports.setInterval = function(callback, repeat) { var timer = new Timer(); + if (process.domain) timer.domain = process.domain; + repeat = ~~repeat; if (repeat < 1 || repeat > TIMEOUT_MAX) { repeat = 1; // schedule on next tick, follows browser behaviour diff --git a/node.gyp b/node.gyp index 7fbf175326..9128041ab7 100644 --- a/node.gyp +++ b/node.gyp @@ -23,6 +23,7 @@ 'lib/cluster.js', 'lib/dgram.js', 'lib/dns.js', + 'lib/domain.js', 'lib/events.js', 'lib/freelist.js', 'lib/fs.js', diff --git a/src/node.js b/src/node.js index 3322df6d25..05c0dd8ecd 100644 --- a/src/node.js +++ b/src/node.js @@ -235,7 +235,13 @@ nextTickQueue = []; try { - for (var i = 0; i < l; i++) q[i](); + for (var i = 0; i < l; i++) { + var tock = q[i]; + var callback = tock.callback; + if (tock.domain) tock.domain.enter(); + callback(); + if (tock.domain) tock.domain.exit(); + } } catch (e) { if (i + 1 < l) { @@ -249,7 +255,9 @@ }; process.nextTick = function(callback) { - nextTickQueue.push(callback); + var tock = { callback: callback }; + if (process.domain) tock.domain = process.domain; + nextTickQueue.push(tock); process._needTickCallback(); }; }; diff --git a/test/simple/test-domain-http-server.js b/test/simple/test-domain-http-server.js new file mode 100644 index 0000000000..bd6336b57f --- /dev/null +++ b/test/simple/test-domain-http-server.js @@ -0,0 +1,115 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +var domain = require('domain'); +var http = require('http'); +var assert = require('assert'); +var common = require('../common.js'); + +var objects = { foo: 'bar', baz: {}, num: 42, arr: [1,2,3] }; +objects.baz.asdf = objects; + +var serverCaught = 0; +var clientCaught = 0 + +var server = http.createServer(function(req, res) { + var dom = domain.create(); + dom.add(req); + dom.add(res); + + dom.on('error', function(er) { + serverCaught++; + console.log('server error', er); + // try to send a 500. If that fails, oh well. + res.writeHead(500, {'content-type':'text/plain'}); + res.end(er.stack || er.message || 'Unknown error'); + }); + + var data; + dom.run(function() { + // Now, an action that has the potential to fail! + // if you request 'baz', then it'll throw a JSON circular ref error. + data = JSON.stringify(objects[req.url.replace(/[^a-z]/g, '')]); + + // this line will throw if you pick an unknown key + assert(data !== undefined, 'Data should not be undefined'); + + res.writeHead(200); + res.end(data); + }); +}); + +server.listen(common.PORT, next); + +function next() { + console.log('listening on localhost:%d', common.PORT); + + // now hit it a few times + var dom = domain.create(); + var requests = 0; + var responses = 0; + + makeReq('/'); + makeReq('/foo'); + makeReq('/arr'); + makeReq('/baz'); + makeReq('/num'); + + function makeReq(p) { + requests++; + + var dom = domain.create(); + dom.on('error', function(er) { + clientCaught++; + console.log('client error', er); + // kill everything. + dom.dispose(); + }); + + var req = http.get({ host: 'localhost', port: common.PORT, path: p }); + dom.add(req); + req.on('response', function(res) { + responses++; + console.error('requests=%d responses=%d', requests, responses); + if (responses === requests) { + console.error('done, closing server'); + // no more coming. + server.close(); + } + + dom.add(res); + var d = ''; + res.on('data', function(c) { + d += c; + }); + res.on('end', function() { + d = JSON.parse(d); + console.log('json!', d); + }); + }); + } +} + +process.on('exit', function() { + assert.equal(serverCaught, 2); + assert.equal(clientCaught, 2); + console.log('ok'); +}); diff --git a/test/simple/test-domain-multi.js b/test/simple/test-domain-multi.js new file mode 100644 index 0000000000..f097c06524 --- /dev/null +++ b/test/simple/test-domain-multi.js @@ -0,0 +1,100 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + + +// Tests of multiple domains happening at once. + +var common = require('../common'); +var assert = require('assert'); +var domain = require('domain'); +var events = require('events'); + +var caughtA = false; +var caughtB = false; +var caughtC = false; + + +var a = domain.create(); +a.enter(); // this will be our "root" domain +a.on('error', function(er) { + caughtA = true; + console.log('This should not happen'); + throw er; +}); + + +var http = require('http'); +var server = http.createServer(function (req, res) { + // child domain. + // implicitly added to a, because we're in a when + // it is created. + var b = domain.create(); + + // treat these EE objects as if they are a part of the b domain + // so, an 'error' event on them propagates to the domain, rather + // than being thrown. + b.add(req); + b.add(res); + + b.on('error', function (er) { + caughtB = true; + console.error('Error encountered', er) + if (res) { + res.writeHead(500); + res.end('An error occurred'); + } + // res.writeHead(500), res.destroy, etc. + server.close(); + }); + + // XXX this bind should not be necessary. + // the write cb behavior in http/net should use an + // event so that it picks up the domain handling. + res.write('HELLO\n', b.bind(function() { + throw new Error('this kills domain B, not A'); + })); + +}).listen(common.PORT); + +var c = domain.create(); +var req = http.get({ host: 'localhost', port: common.PORT }) + +// add the request to the C domain +c.add(req); + +req.on('response', function(res) { + console.error('got response'); + // add the response object to the C domain + c.add(res); + res.pipe(process.stdout); +}); + +c.on('error', function(er) { + caughtC = true; + console.error('Error on c', er.message); +}); + +process.on('exit', function() { + assert.equal(caughtA, false); + assert.equal(caughtB, true) + assert.equal(caughtC, true) + console.log('ok - Errors went where they were supposed to go'); +}); diff --git a/test/simple/test-domain.js b/test/simple/test-domain.js new file mode 100644 index 0000000000..e20868ed0f --- /dev/null +++ b/test/simple/test-domain.js @@ -0,0 +1,198 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + + +// Simple tests of most basic domain functionality. + +var common = require('../common'); +var assert = require('assert'); +var domain = require('domain'); +var events = require('events'); +var caught = 0; +var expectCaught = 8; + +var d = new domain.Domain(); +var e = new events.EventEmitter(); + +d.on('error', function(er) { + console.error('caught', er); + switch (er.message) { + case 'emitted': + assert.equal(er.domain, d); + assert.equal(er.domain_emitter, e); + assert.equal(er.domain_thrown, false); + break; + + case 'bound': + assert.ok(!er.domain_emitter); + assert.equal(er.domain, d); + assert.equal(er.domain_bound, fn); + assert.equal(er.domain_thrown, false); + break; + + case 'thrown': + assert.ok(!er.domain_emitter); + assert.equal(er.domain, d); + assert.equal(er.domain_thrown, true); + break; + + case "ENOENT, open 'this file does not exist'": + assert.equal(er.domain, d); + assert.equal(er.domain_thrown, false); + assert.equal(typeof er.domain_bound, 'function'); + assert.ok(!er.domain_emitter); + assert.equal(er.code, 'ENOENT'); + assert.equal(er.path, 'this file does not exist'); + assert.equal(typeof er.errno, 'number'); + break; + + case "ENOENT, open 'stream for nonexistent file'": + assert.equal(typeof er.errno, 'number'); + assert.equal(er.code, 'ENOENT'); + assert.equal(er.path, 'stream for nonexistent file'); + assert.equal(er.domain, d); + assert.equal(er.domain_emitter, fst); + assert.ok(!er.domain_bound); + assert.equal(er.domain_thrown, false); + break; + + case 'implicit': + assert.equal(er.domain_emitter, implicit); + assert.equal(er.domain, d); + assert.equal(er.domain_thrown, false); + assert.ok(!er.domain_bound); + break; + + case 'implicit timer': + assert.equal(er.domain, d); + assert.equal(er.domain_thrown, true); + assert.ok(!er.domain_emitter); + assert.ok(!er.domain_bound); + break; + + case 'Cannot call method \'isDirectory\' of undefined': + assert.equal(er.domain, d); + assert.ok(!er.domain_emitter); + assert.ok(!er.domain_bound); + break; + + default: + console.error('unexpected error, throwing %j', er.message); + throw er; + } + + caught++; +}); + +process.on('exit', function() { + console.error('exit'); + assert.equal(caught, expectCaught); + console.log('ok'); +}); + + + +// Event emitters added to the domain have their errors routed. +d.add(e); +e.emit('error', new Error('emitted')); + + + +// get rid of the `if (er) return cb(er)` malarky, by intercepting +// the cb functions to the domain, and using the intercepted function +// as a callback instead. +function fn(er) { + throw new Error('This function should never be called!'); + process.exit(1); +} + +var bound = d.intercept(fn); +bound(new Error('bound')); + + + +// throwing in a bound fn is also caught, +// even if it's asynchronous, by hitting the +// global uncaughtException handler. This doesn't +// require interception, since throws are always +// caught by the domain. +function thrower() { + throw new Error('thrown'); +} +setTimeout(d.bind(thrower), 100); + + + +// Pass an intercepted function to an fs operation that fails. +var fs = require('fs'); +fs.open('this file does not exist', 'r', d.intercept(function(er) { + console.error('should not get here!', er); + throw new Error('should not get here!'); +}, true)); + + + +// catch thrown errors no matter how many times we enter the event loop +// this only uses implicit binding, except for the first function +// passed to d.run(). The rest are implicitly bound by virtue of being +// set up while in the scope of the d domain. +d.run(function() { + process.nextTick(function() { + var i = setInterval(function () { + clearInterval(i); + setTimeout(function() { + fs.stat('this file does not exist', function(er, stat) { + // uh oh! stat isn't set! + // pretty common error. + console.log(stat.isDirectory()); + }); + }); + }); + }); +}); + + + +// implicit addition by being created within a domain-bound context. +var implicit; + +d.run(function() { + implicit = new events.EventEmitter; +}); + +setTimeout(function() { + // escape from the domain, but implicit is still bound to it. + implicit.emit('error', new Error('implicit')); +}, 10); + + + +// implicit addition of a timer created within a domain-bound context. +d.run(function() { + setTimeout(function() { + throw new Error('implicit timer'); + }); +}); + + + +var fst = fs.createReadStream('stream for nonexistent file') +d.add(fst) From 10ce3d129d4e466842a03545fdfdfc011e0b9fba Mon Sep 17 00:00:00 2001 From: isaacs Date: Fri, 13 Apr 2012 16:27:23 -0700 Subject: [PATCH 11/13] Domain hooks in ReqWrap and MakeCallback --- src/node.cc | 42 ++++++++++++++++++++++++++++++++++++++++++ src/node.js | 5 ++++- src/req_wrap.h | 21 +++++++++++++++++++++ 3 files changed, 67 insertions(+), 1 deletion(-) diff --git a/src/node.cc b/src/node.cc index c5e15161b4..7bfcdb8623 100644 --- a/src/node.cc +++ b/src/node.cc @@ -109,6 +109,11 @@ static Persistent listeners_symbol; static Persistent uncaught_exception_symbol; static Persistent emit_symbol; +static Persistent domain_symbol; +static Persistent enter_symbol; +static Persistent exit_symbol; +static Persistent disposed_symbol; + static bool print_eval = false; static bool force_repl = false; @@ -1017,10 +1022,47 @@ MakeCallback(const Handle object, TryCatch try_catch; + if (domain_symbol.IsEmpty()) { + domain_symbol = NODE_PSYMBOL("domain"); + enter_symbol = NODE_PSYMBOL("enter"); + exit_symbol = NODE_PSYMBOL("exit"); + disposed_symbol = NODE_PSYMBOL("_disposed"); + } + + Local domain_v = object->Get(domain_symbol); + Local domain; + Local enter; + Local exit; + if (!domain_v->IsUndefined()) { + domain = domain_v->ToObject(); + if (domain->Get(disposed_symbol)->BooleanValue()) { + // domain has been disposed of. + return Undefined(); + } + enter = Local::Cast(domain->Get(enter_symbol)); + enter->Call(domain, 0, NULL); + } + + if (try_catch.HasCaught()) { + FatalException(try_catch); + return Undefined(); + } + Local ret = callback->Call(object, argc, argv); if (try_catch.HasCaught()) { FatalException(try_catch); + return Undefined(); + } + + if (!domain_v->IsUndefined()) { + exit = Local::Cast(domain->Get(exit_symbol)); + exit->Call(domain, 0, NULL); + } + + if (try_catch.HasCaught()) { + FatalException(try_catch); + return Undefined(); } return scope.Close(ret); diff --git a/src/node.js b/src/node.js index 05c0dd8ecd..cc5e7b65da 100644 --- a/src/node.js +++ b/src/node.js @@ -238,7 +238,10 @@ for (var i = 0; i < l; i++) { var tock = q[i]; var callback = tock.callback; - if (tock.domain) tock.domain.enter(); + if (tock.domain) { + if (tock.domain._disposed) continue; + tock.domain.enter(); + } callback(); if (tock.domain) tock.domain.exit(); } diff --git a/src/req_wrap.h b/src/req_wrap.h index b8530e8950..c478ce0cdb 100644 --- a/src/req_wrap.h +++ b/src/req_wrap.h @@ -24,14 +24,35 @@ namespace node { +static v8::Persistent process_symbol; +static v8::Persistent domain_symbol; + template class ReqWrap { public: ReqWrap() { v8::HandleScope scope; object_ = v8::Persistent::New(v8::Object::New()); + + // TODO: grab a handle to the current process.domain + if (process_symbol.IsEmpty()) { + process_symbol = NODE_PSYMBOL("process"); + domain_symbol = NODE_PSYMBOL("domain"); + } + + v8::Local domain = v8::Context::GetCurrent() + ->Global() + ->Get(process_symbol) + ->ToObject() + ->Get(domain_symbol); + + if (!domain->IsUndefined()) { + // fprintf(stderr, "setting domain on ReqWrap\n"); + object_->Set(domain_symbol, domain); + } } + ~ReqWrap() { // Assert that someone has called Dispatched() assert(req_.data == this); From 45c1d4f96f48c30bd3036938fb922e050e76c2e0 Mon Sep 17 00:00:00 2001 From: isaacs Date: Tue, 10 Apr 2012 14:59:21 -0700 Subject: [PATCH 12/13] Add switches to http_simple bench to use domains --- benchmark/http.sh | 4 +++- benchmark/http_simple.js | 20 ++++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/benchmark/http.sh b/benchmark/http.sh index 22b4b7c8c6..40d0c714c2 100755 --- a/benchmark/http.sh +++ b/benchmark/http.sh @@ -4,7 +4,9 @@ sudo sysctl -w net.inet.ip.portrange.first=12000 sudo sysctl -w net.inet.tcp.msl=1000 sudo sysctl -w kern.maxfiles=1000000 kern.maxfilesperproc=1000000 ulimit -n 100000 + ./node benchmark/http_simple.js || exit 1 & sleep 1 -ab -n 30000 -c 100 http://127.0.0.1:8000/bytes/123 | grep Req + +ab -n 30000 -c 100 http://127.0.0.1:8000/${TYPE:-bytes}/${LENGTH:-1024} | grep Req killall node diff --git a/benchmark/http_simple.js b/benchmark/http_simple.js index 74ba4d7768..bd0a1c8364 100644 --- a/benchmark/http_simple.js +++ b/benchmark/http_simple.js @@ -14,7 +14,27 @@ for (var i = 0; i < 20*1024; i++) { stored = {}; storedBuffer = {}; +var useDomains = process.env.NODE_USE_DOMAINS; + +// set up one global domain. +if (useDomains) { + var domain = require('domain'); + var gdom = domain.create(); + gdom.on('error', function(er) { + console.log('Error on global domain', er); + throw er; + }); + gdom.enter(); +} + var server = http.createServer(function (req, res) { + + if (useDomains) { + var dom = domain.create(); + dom.add(req); + dom.add(res); + } + var commands = req.url.split("/"); var command = commands[1]; var body = ""; From d4ed2e61f7acf9359c6c5c74674677f8f0c0db77 Mon Sep 17 00:00:00 2001 From: isaacs Date: Thu, 12 Apr 2012 08:16:30 -0700 Subject: [PATCH 13/13] Add Todo comments about domain-ifying crypto --- src/node_crypto.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 5f6594b8e5..c05e8e3361 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -864,6 +864,9 @@ int Connection::SelectSNIContextCallback_(SSL *s, int *ad, void* arg) { Local callback = *p->sniCallback_; // Call it + // + // XXX There should be an object connected to this that + // we can attach a domain onto. Local ret; ret = Local::New(MakeCallback(Context::GetCurrent()->Global(), callback, ARRAY_SIZE(argv), argv)); @@ -4123,6 +4126,8 @@ EIO_PBKDF2After(uv_work_t* req) { argv[1] = Local::New(Undefined()); } + // XXX There should be an object connected to this that + // we can attach a domain onto. MakeCallback(Context::GetCurrent()->Global(), request->callback, ARRAY_SIZE(argv), argv); @@ -4313,6 +4318,8 @@ void RandomBytesAfter(uv_work_t* work_req) { Local argv[2]; RandomBytesCheck(req, argv); + // XXX There should be an object connected to this that + // we can attach a domain onto. MakeCallback(Context::GetCurrent()->Global(), req->callback_, ARRAY_SIZE(argv), argv);