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

util: use V8 C++ API for inspecting Promises #12254

Merged
merged 1 commit into from
Apr 8, 2017
+54 −38
Merged

util: use V8 C++ API for inspecting Promises #12254

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
62 changes: 24 additions & 38 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,16 +302,6 @@ function ensureDebugIsInitialized() {
}


function inspectPromise(p) {
// Only create a mirror if the object is a Promise.
if (!binding.isPromise(p))
return null;
ensureDebugIsInitialized();
const mirror = Debug.MakeMirror(p, true);
return {status: mirror.status(), value: mirror.promiseValue().value_};
}


function formatValue(ctx, value, recurseTimes) {
if (ctx.showProxy &&
((typeof value === 'object' && value !== null) ||
Expand Down Expand Up @@ -527,30 +517,25 @@ function formatValue(ctx, value, recurseTimes) {
'byteOffset',
'buffer');
}
} else if (binding.isPromise(value)) {
braces = ['{', '}'];
formatter = formatPromise;
} else if (binding.isMapIterator(value)) {
constructor = { name: 'MapIterator' };
braces = ['{', '}'];
empty = false;
formatter = formatCollectionIterator;
} else if (binding.isSetIterator(value)) {
constructor = { name: 'SetIterator' };
braces = ['{', '}'];
empty = false;
formatter = formatCollectionIterator;
} else {
var promiseInternals = inspectPromise(value);
if (promiseInternals) {
braces = ['{', '}'];
formatter = formatPromise;
} else {
if (binding.isMapIterator(value)) {
constructor = { name: 'MapIterator' };
braces = ['{', '}'];
empty = false;
formatter = formatCollectionIterator;
} else if (binding.isSetIterator(value)) {
constructor = { name: 'SetIterator' };
braces = ['{', '}'];
empty = false;
formatter = formatCollectionIterator;
} else {
// Unset the constructor to prevent "Object {...}" for ordinary objects.
if (constructor && constructor.name === 'Object')
constructor = null;
braces = ['{', '}'];
empty = true; // No other data than keys.
}
}
// Unset the constructor to prevent "Object {...}" for ordinary objects.
if (constructor && constructor.name === 'Object')
constructor = null;
braces = ['{', '}'];
empty = true; // No other data than keys.
}

empty = empty === true && keys.length === 0;
Expand Down Expand Up @@ -779,14 +764,15 @@ function formatCollectionIterator(ctx, value, recurseTimes, visibleKeys, keys) {
}

function formatPromise(ctx, value, recurseTimes, visibleKeys, keys) {
var output = [];
var internals = inspectPromise(value);
if (internals.status === 'pending') {
const output = [];
const [state, result] = binding.getPromiseDetails(value);

if (state === binding.kPending) {
output.push('<pending>');
} else {
var nextRecurseTimes = recurseTimes === null ? null : recurseTimes - 1;
var str = formatValue(ctx, internals.value, nextRecurseTimes);
if (internals.status === 'rejected') {
var str = formatValue(ctx, result, nextRecurseTimes);
if (state === binding.kRejected) {
Copy link
Member

Choose a reason for hiding this comment

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

Also fyi @Fishrock123, this seems like a cleaner way to inspect Promise states than what the abort-on-rejected PR currently does…

Copy link
Contributor

Choose a reason for hiding this comment

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

Errr, this allows you to get the actual result object e.g. an error without going through the Debug API? I was told that was not possible to do synchronously.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Fishrock123 this set of APIs is relatively new, having been added in V8 5.7 in v8/v8@2843258. That might be why.

output.push('<rejected> ' + str);
} else {
output.push(str);
Expand Down
30 changes: 30 additions & 0 deletions src/node_util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ using v8::Integer;
using v8::Local;
using v8::Object;
using v8::Private;
using v8::Promise;
using v8::Proxy;
using v8::Value;

Expand Down Expand Up @@ -43,6 +44,24 @@ using v8::Value;
VALUE_METHOD_MAP(V)
#undef V

static void GetPromiseDetails(const FunctionCallbackInfo<Value>& args) {
// Return undefined if it's not a Promise.
if (!args[0]->IsPromise())
return;

auto isolate = args.GetIsolate();

Local<Promise> promise = args[0].As<Promise>();
Local<Array> ret = Array::New(isolate, 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

might it be better to use Object and name the properties? might be easier if we want to extend with additional metadata, and would allow the JavaScript to be de-coupled from the order of items in the array.

Copy link
Member Author

Choose a reason for hiding this comment

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

Besides the fact that this is simpler to implement, it is also the approach taken by the existing GetProxyDetails. Plus, I don't think it is very likely for promises to change its internal properties anytime soon.


int state = promise->State();
ret->Set(0, Integer::New(isolate, state));
if (state != Promise::PromiseState::kPending)
ret->Set(1, promise->Result());

args.GetReturnValue().Set(ret);
}

static void GetProxyDetails(const FunctionCallbackInfo<Value>& args) {
// Return undefined if it's not a proxy.
if (!args[0]->IsProxy())
Expand Down Expand Up @@ -148,8 +167,19 @@ void Initialize(Local<Object> target,
Integer::NewFromUnsigned(env->isolate(), NODE_PUSH_VAL_TO_ARRAY_MAX),
v8::ReadOnly).FromJust();

#define V(name) \
target->Set(context, \
FIXED_ONE_BYTE_STRING(env->isolate(), #name), \
Integer::New(env->isolate(), Promise::PromiseState::name)) \
.FromJust()
V(kPending);
V(kFulfilled);
V(kRejected);
#undef V

env->SetMethod(target, "getHiddenValue", GetHiddenValue);
env->SetMethod(target, "setHiddenValue", SetHiddenValue);
env->SetMethod(target, "getPromiseDetails", GetPromiseDetails);
env->SetMethod(target, "getProxyDetails", GetProxyDetails);

env->SetMethod(target, "startSigintWatchdog", StartSigintWatchdog);
Expand Down