Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement unhandled rejection tracking #758

Closed
wants to merge 1 commit into from
Closed

Implement unhandled rejection tracking #758

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ namespace node {
V(module_load_list_array, v8::Array) \
V(pipe_constructor_template, v8::FunctionTemplate) \
V(process_object, v8::Object) \
V(promise_reject_function, v8::Function) \
V(script_context_constructor_template, v8::FunctionTemplate) \
V(script_data_constructor_function, v8::Function) \
V(secure_context_constructor_template, v8::FunctionTemplate) \
Expand Down
56 changes: 56 additions & 0 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ using v8::Message;
using v8::Number;
using v8::Object;
using v8::ObjectTemplate;
using v8::Promise;
using v8::PromiseRejectMessage;
using v8::PropertyCallbackInfo;
using v8::String;
using v8::TryCatch;
Expand Down Expand Up @@ -982,6 +984,37 @@ void SetupNextTick(const FunctionCallbackInfo<Value>& args) {
FIXED_ONE_BYTE_STRING(args.GetIsolate(), "_setupNextTick"));
}

void PromiseRejectCallback(PromiseRejectMessage message) {
Local<Promise> promise = message.GetPromise();
Isolate* isolate = promise->GetIsolate();
Local<Value> value = message.GetValue();
Local<Integer> event = Integer::New(isolate, message.GetEvent());

Environment* env = Environment::GetCurrent(isolate);
Local<Function> callback = env->promise_reject_function();

if (value.IsEmpty())
value = Undefined(isolate);

Local<Value> args[] = { event, promise, value };
Local<Object> process = env->process_object();

callback->Call(process, ARRAY_SIZE(args), args);
}

void SetupPromises(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Isolate* isolate = env->isolate();

CHECK(args[0]->IsFunction());

isolate->SetPromiseRejectCallback(PromiseRejectCallback);
env->set_promise_reject_function(args[0].As<Function>());

env->process_object()->Delete(
FIXED_ONE_BYTE_STRING(args.GetIsolate(), "_setupPromises"));
}


Handle<Value> MakeCallback(Environment* env,
Handle<Value> recv,
Expand Down Expand Up @@ -2572,6 +2605,14 @@ void StopProfilerIdleNotifier(const FunctionCallbackInfo<Value>& args) {
obj->ForceSet(OneByteString(env->isolate(), str), var, v8::ReadOnly); \
} while (0)

#define READONLY_DONT_ENUM_PROPERTY(obj, str, var) \
do { \
obj->ForceSet(OneByteString(env->isolate(), str), \
var, \
static_cast<v8::PropertyAttribute>(v8::ReadOnly | \
v8::DontEnum)); \
} while (0)


void SetupProcessObject(Environment* env,
int argc,
Expand Down Expand Up @@ -2632,6 +2673,20 @@ void SetupProcessObject(Environment* env,
"modules",
FIXED_ONE_BYTE_STRING(env->isolate(), node_modules_version));

// process._promiseRejectEvent
Local<Object> promiseRejectEvent = Object::New(env->isolate());
READONLY_DONT_ENUM_PROPERTY(process,
"_promiseRejectEvent",
promiseRejectEvent);
READONLY_PROPERTY(promiseRejectEvent,
"unhandled",
Integer::New(env->isolate(),
v8::kPromiseRejectWithNoHandler));
READONLY_PROPERTY(promiseRejectEvent,
"handled",
Integer::New(env->isolate(),
v8::kPromiseHandlerAddedAfterReject));

#if HAVE_OPENSSL
// Stupid code to slice out the version string.
{ // NOLINT(whitespace/braces)
Expand Down Expand Up @@ -2790,6 +2845,7 @@ void SetupProcessObject(Environment* env,
env->SetMethod(process, "_linkedBinding", LinkedBinding);

env->SetMethod(process, "_setupNextTick", SetupNextTick);
env->SetMethod(process, "_setupPromises", SetupPromises);
env->SetMethod(process, "_setupDomainUse", SetupDomainUse);

// pre-set _events object for faster emit checks
Expand Down
58 changes: 57 additions & 1 deletion src/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
startup.processAssert();
startup.processConfig();
startup.processNextTick();
startup.processPromises();
startup.processStdio();
startup.processKillAndExit();
startup.processSignalHandlers();
Expand Down Expand Up @@ -264,8 +265,11 @@
});
};

var addPendingUnhandledRejection;
var hasBeenNotifiedProperty = new WeakMap();
startup.processNextTick = function() {
var nextTickQueue = [];
var pendingUnhandledRejections = [];
var microtasksScheduled = false;

// Used to run V8's micro task queue.
Expand Down Expand Up @@ -318,7 +322,8 @@
microtasksScheduled = false;
_runMicrotasks();

if (tickInfo[kIndex] < tickInfo[kLength])
if (tickInfo[kIndex] < tickInfo[kLength] ||
emitPendingUnhandledRejections())
scheduleMicrotasks();
}

Expand Down Expand Up @@ -388,6 +393,57 @@
nextTickQueue.push(obj);
tickInfo[kLength]++;
}

function emitPendingUnhandledRejections() {
var hadListeners = false;
while (pendingUnhandledRejections.length > 0) {
var promise = pendingUnhandledRejections.shift();
var reason = pendingUnhandledRejections.shift();
if (hasBeenNotifiedProperty.get(promise) === false) {
hasBeenNotifiedProperty.set(promise, true);
if (!process.emit('unhandledRejection', reason, promise)) {
Copy link

Choose a reason for hiding this comment

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

Why not just directly emit "uncaughtException"? Is't it very similar?

Copy link
Contributor

Choose a reason for hiding this comment

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

Rejections can be handled asynchronously after

Copy link
Member

Choose a reason for hiding this comment

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

No, it is not. Choosing to deal with unhandled rejections like uncaught exceptions is a very opinionated choice.

I'm not saying I agree or disagree with it but it is a breaking change for many users regardless - and not one we should easily introduce. The current plan is to introduce these events now and then choose a default action later - one such default action is to throw the error for "uncaughtException" events to deal with it - there are people who like it and people who object to it. However, at first we need the very basic ability to deal with these events so this pull request makes the minimal changes required that don't make any breaking changes to existing io.js code.

Copy link

Choose a reason for hiding this comment

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

Thanks, it is reasonable

Copy link
Member

Choose a reason for hiding this comment

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

@golyshevd I've actually since opened an issue about this where you are welcome to argue your stance: #830

// Nobody is listening.
// TODO(petkaantonov) Take some default action, see #830
} else
hadListeners = true;
}
}
return hadListeners;
}

addPendingUnhandledRejection = function(promise, reason) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do it this way instead of just function addPendingUnhandledRejection() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The promiseSetup needs to access it as well

pendingUnhandledRejections.push(promise, reason);
scheduleMicrotasks();
};
};

startup.processPromises = function() {
var promiseRejectEvent = process._promiseRejectEvent;
Copy link
Member

Choose a reason for hiding this comment

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

Using an object is not wrong but I would just have created kPromiseRejectWithNoHandler and kPromiseHandlerAddedAfterReject properties on the process object, then assign them to locals here. That's how e.g. lib/_http_common.js operates.


function unhandledRejection(promise, reason) {
hasBeenNotifiedProperty.set(promise, false);
addPendingUnhandledRejection(promise, reason);
}

function rejectionHandled(promise) {
var hasBeenNotified = hasBeenNotifiedProperty.get(promise);
if (hasBeenNotified !== undefined) {
hasBeenNotifiedProperty.delete(promise);
if (hasBeenNotified === true)
process.emit('rejectionHandled', promise);
}
}

process._setupPromises(function(event, promise, reason) {
if (event === promiseRejectEvent.unhandled)
unhandledRejection(promise, reason);
else if (event === promiseRejectEvent.handled)
process.nextTick(function() {
rejectionHandled(promise);
});
else
NativeModule.require('assert').fail('unexpected PromiseRejectEvent');
});
};

function evalScript(name) {
Expand Down
Loading