-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
weak handles and promises in progress stuff #5292
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,7 @@ | |
#include "req-wrap.h" | ||
#include "req-wrap-inl.h" | ||
#include "string_bytes.h" | ||
#include "track-promise.h" | ||
#include "util.h" | ||
#include "uv.h" | ||
#include "libplatform/libplatform.h" | ||
|
@@ -104,6 +105,7 @@ using v8::Array; | |
using v8::ArrayBuffer; | ||
using v8::Boolean; | ||
using v8::Context; | ||
using v8::Debug; | ||
using v8::EscapableHandleScope; | ||
using v8::Exception; | ||
using v8::Function; | ||
|
@@ -118,10 +120,12 @@ using v8::Locker; | |
using v8::MaybeLocal; | ||
using v8::Message; | ||
using v8::Name; | ||
using v8::NativeWeakMap; | ||
using v8::Null; | ||
using v8::Number; | ||
using v8::Object; | ||
using v8::ObjectTemplate; | ||
using v8::Persistent; | ||
using v8::Promise; | ||
using v8::PromiseRejectMessage; | ||
using v8::PropertyCallbackInfo; | ||
|
@@ -167,6 +171,10 @@ static const char* icu_data_dir = nullptr; | |
// used by C++ modules as well | ||
bool no_deprecation = false; | ||
|
||
// Unhandled promises tracking | ||
Persistent<NativeWeakMap>* remainingURs; | ||
Persistent<Object>* first_ur_key; | ||
|
||
#if HAVE_OPENSSL && NODE_FIPS_MODE | ||
// used by crypto module | ||
bool enable_fips_crypto = false; | ||
|
@@ -1138,14 +1146,59 @@ void PromiseRejectCallback(PromiseRejectMessage message) { | |
callback->Call(process, arraysize(args), args); | ||
} | ||
|
||
void OnPromiseGC(const FunctionCallbackInfo<Value>& args) { | ||
Environment* env = Environment::GetCurrent(args); | ||
|
||
CHECK(args[0]->IsObject()); | ||
Local<Object> promise = args[0].As<Object>(); | ||
|
||
TrackPromise::New(env->isolate(), promise); | ||
|
||
HandleScope scope(env->isolate()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the HandleScope? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm pretty sure I needed it to make new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. JS -> C++ callbacks have an implicit HandleScope, you don't need to create one yourself. |
||
Local<NativeWeakMap> l_remainingURs = | ||
Local<NativeWeakMap>::New(env->isolate(), *remainingURs); | ||
Local<Object> l_first_ur_key = | ||
Local<Object>::New(env->isolate(), *first_ur_key); | ||
if (!l_remainingURs->Has(l_first_ur_key)) { | ||
l_remainingURs->Set(l_first_ur_key, promise); | ||
} | ||
} | ||
|
||
void UntrackPromise(const FunctionCallbackInfo<Value>& args) { | ||
Environment* env = Environment::GetCurrent(args); | ||
|
||
CHECK(args[0]->IsObject()); | ||
Local<Value> promise = args[0].As<Value>(); | ||
|
||
HandleScope scope(env->isolate()); | ||
Local<NativeWeakMap> l_remainingURs = | ||
Local<NativeWeakMap>::New(env->isolate(), *remainingURs); | ||
Local<Object> l_first_ur_key = | ||
Local<Object>::New(env->isolate(), *first_ur_key); | ||
if (l_remainingURs->Get(l_first_ur_key) == promise) { | ||
l_remainingURs->Delete(l_first_ur_key); | ||
} | ||
} | ||
|
||
void SetupPromises(const FunctionCallbackInfo<Value>& args) { | ||
Environment* env = Environment::GetCurrent(args); | ||
Isolate* isolate = env->isolate(); | ||
|
||
HandleScope scope(isolate); | ||
remainingURs = new Persistent<NativeWeakMap>(isolate, | ||
NativeWeakMap::New(isolate)); | ||
first_ur_key = new Persistent<Object>(isolate, Object::New(isolate)); | ||
|
||
CHECK(args[0]->IsFunction()); | ||
CHECK(args[1]->IsFunction()); | ||
CHECK(args[2]->IsObject()); | ||
|
||
isolate->SetPromiseRejectCallback(PromiseRejectCallback); | ||
env->set_promise_reject_function(args[0].As<Function>()); | ||
env->set_promise_unhandled_rejection(args[1].As<Function>()); | ||
|
||
env->SetMethod(args[2].As<Object>(), "onPromiseGC", OnPromiseGC); | ||
env->SetMethod(args[2].As<Object>(), "untrackPromise", UntrackPromise); | ||
|
||
env->process_object()->Delete( | ||
env->context(), | ||
|
@@ -1574,8 +1627,26 @@ void AppendExceptionLine(Environment* env, | |
PrintErrorString("\n%s", arrow); | ||
} | ||
|
||
void ReportPromiseRejection(Isolate* isolate, Local<Object> object) { | ||
Environment* env = Environment::GetCurrent(isolate); | ||
Local<Function> fn = env->promise_unhandled_rejection(); | ||
|
||
if (!fn.IsEmpty()) { | ||
HandleScope scope(isolate); | ||
Local<Value> promise = object.As<Value>(); | ||
Local<Value> internalProps = | ||
Debug::GetInternalProperties(isolate, | ||
promise).ToLocalChecked().As<Value>(); | ||
|
||
Local<Value> err = fn->Call(env->context(), Null(env->isolate()), 1, | ||
&internalProps).ToLocalChecked(); | ||
|
||
ReportException(env, err, Exception::CreateMessage(isolate, err)); | ||
} | ||
exit(1); | ||
} | ||
|
||
static void ReportException(Environment* env, | ||
void ReportException(Environment* env, | ||
Local<Value> er, | ||
Local<Message> message) { | ||
HandleScope scope(env->isolate()); | ||
|
@@ -4323,6 +4394,16 @@ static void StartNodeInstance(void* arg) { | |
} while (more == true); | ||
} | ||
|
||
HandleScope scope(isolate); | ||
Local<NativeWeakMap> l_remainingURs = | ||
Local<NativeWeakMap>::New(isolate, *remainingURs); | ||
Local<Object> l_first_ur_key = | ||
Local<Object>::New(env->isolate(), *first_ur_key); | ||
if (l_remainingURs->Has(l_first_ur_key)) { | ||
Local<Object> firstUR = l_remainingURs->Get(l_first_ur_key).As<Object>(); | ||
ReportPromiseRejection(isolate, firstUR); | ||
} | ||
|
||
env->set_trace_sync_io(false); | ||
|
||
int exit_code = EmitExit(env); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
#include "env.h" | ||
#include "env-inl.h" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not following why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @trevnorris Actually, it seemed to function without anything being included. I really don't know why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems you have a copy of the If nothing else, other than this files, needs to know about the |
||
#include "node_internals.h" | ||
|
||
namespace node { | ||
|
||
using v8::Exception; | ||
using v8::Function; | ||
using v8::HandleScope; | ||
using v8::Isolate; | ||
using v8::Local; | ||
using v8::Object; | ||
using v8::Persistent; | ||
using v8::Value; | ||
using v8::WeakCallbackData; | ||
|
||
typedef void (*FreeCallback)(Local<Object> object, Local<Function> fn); | ||
|
||
class TrackPromise { | ||
public: | ||
static TrackPromise* New(Isolate* isolate, Local<Object> object); | ||
inline Persistent<Object>* persistent(); | ||
private: | ||
static void WeakCallback(const WeakCallbackData<Object, TrackPromise>&); | ||
inline void WeakCallback(Isolate* isolate, Local<Object> object); | ||
inline TrackPromise(Isolate* isolate, Local<Object> object); | ||
~TrackPromise(); | ||
Persistent<Object> persistent_; | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bnoordhuis I'm not sure to get rid of this; it's duplicated in the header but that doesn't seem to work? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's because you don't |
||
|
||
|
||
TrackPromise* TrackPromise::New(Isolate* isolate, | ||
Local<Object> object) { | ||
return new TrackPromise(isolate, object); | ||
} | ||
|
||
|
||
Persistent<Object>* TrackPromise::persistent() { | ||
return &persistent_; | ||
} | ||
|
||
|
||
TrackPromise::TrackPromise(Isolate* isolate, | ||
Local<Object> object) | ||
: persistent_(isolate, object) { | ||
persistent_.SetWeak(this, WeakCallback); | ||
persistent_.MarkIndependent(); | ||
} | ||
|
||
|
||
TrackPromise::~TrackPromise() { | ||
persistent_.Reset(); | ||
} | ||
|
||
|
||
void TrackPromise::WeakCallback( | ||
const WeakCallbackData<Object, TrackPromise>& data) { | ||
data.GetParameter()->WeakCallback(data.GetIsolate(), data.GetValue()); | ||
} | ||
|
||
|
||
void TrackPromise::WeakCallback(Isolate* isolate, Local<Object> object) { | ||
node::ReportPromiseRejection(isolate, object); | ||
delete this; | ||
} | ||
|
||
} // namespace node |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
#ifndef SRC_TRACK_PROMISE_H_ | ||
#define SRC_TRACK_PROMISE_H_ | ||
|
||
#include "v8.h" | ||
|
||
namespace node { | ||
|
||
class Environment; | ||
|
||
class TrackPromise { | ||
public: | ||
TrackPromise(v8::Isolate* isolate, v8::Local<v8::Object> object); | ||
virtual ~TrackPromise(); | ||
|
||
static TrackPromise* New(v8::Isolate* isolate, | ||
v8::Local<v8::Object> object); | ||
|
||
inline v8::Persistent<v8::Object>& persistent(); | ||
|
||
inline void WeakCallback( | ||
const v8::WeakCallbackData<v8::Object, TrackPromise>& data); | ||
|
||
private: | ||
inline void WeakCallback(v8::Isolate* isolate, v8::Local<v8::Object> object); | ||
|
||
v8::Persistent<v8::Object> persistent_; | ||
}; | ||
|
||
} // namespace node | ||
|
||
#endif // SRC_TRACK_PROMISE_H_ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
'use strict'; | ||
require('../common'); | ||
|
||
new Promise(function(res, rej) { | ||
consol.log('One'); | ||
}); | ||
|
||
new Promise(function(res, rej) { | ||
consol.log('Two'); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
*test*message*promise_fast_reject.js:* | ||
consol.log('One'); | ||
^ | ||
|
||
ReferenceError: consol is not defined | ||
at *test*message*promise_fast_reject.js:*:* | ||
at Object.<anonymous> (*test*message*promise_fast_reject.js:*:*) | ||
at Module._compile (module.js:*:*) | ||
at Object.Module._extensions..js (module.js:*:*) | ||
at Module.load (module.js:*:*) | ||
at tryModuleLoad (module.js:*:*) | ||
at Function.Module._load (module.js:*:*) | ||
at Function.Module.runMain (module.js:*:*) | ||
at startup (node.js:*:*) | ||
at node.js:*:* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be part of
Environment
. Also, why are these pointers?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.Will do.
2.
Persistent
s appear to return pointers?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand 2 but
Persistent
is just a value type. You assign it aLocal
through the.Reset()
method. Or is your question how to turn aPersistent
into aLocal
again?