Skip to content

Commit

Permalink
Fix promise reject issue (denoland#936)
Browse files Browse the repository at this point in the history
  • Loading branch information
kevinkassimo authored and ry committed Oct 12, 2018
1 parent c9f95d5 commit 45d3b89
Show file tree
Hide file tree
Showing 14 changed files with 299 additions and 6 deletions.
1 change: 1 addition & 0 deletions BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ ts_sources = [
"js/os.ts",
"js/platform.ts",
"js/plugins.d.ts",
"js/promise_util.ts",
"js/read_dir.ts",
"js/read_file.ts",
"js/read_link.ts",
Expand Down
17 changes: 17 additions & 0 deletions js/libdeno.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@ import { globalEval } from "./global_eval";

// The libdeno functions are moved so that users can't access them.
type MessageCallback = (msg: Uint8Array) => void;
export type PromiseRejectEvent =
| "RejectWithNoHandler"
| "HandlerAddedAfterReject"
| "ResolveAfterResolved"
| "RejectAfterResolved";

interface Libdeno {
recv(cb: MessageCallback): void;

Expand All @@ -20,6 +26,17 @@ interface Libdeno {
) => void
) => void;

setPromiseRejectHandler: (
handler: (
error: Error | string,
event: PromiseRejectEvent,
/* tslint:disable-next-line:no-any */
promise: Promise<any>
) => void
) => void;

setPromiseErrorExaminer: (handler: () => boolean) => void;

mainSource: string;
mainSourceMap: RawSourceMap;
}
Expand Down
3 changes: 3 additions & 0 deletions js/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { DenoCompiler } from "./compiler";
import { libdeno } from "./libdeno";
import { args } from "./deno";
import { sendSync, handleAsyncMsgFromRust } from "./dispatch";
import { promiseErrorExaminer, promiseRejectHandler } from "./promise_util";

function sendStart(): msg.StartRes {
const builder = new flatbuffers.Builder();
Expand Down Expand Up @@ -39,6 +40,8 @@ function onGlobalError(
export default function denoMain() {
libdeno.recv(handleAsyncMsgFromRust);
libdeno.setGlobalErrorHandler(onGlobalError);
libdeno.setPromiseRejectHandler(promiseRejectHandler);
libdeno.setPromiseErrorExaminer(promiseErrorExaminer);
const compiler = DenoCompiler.instance();

// First we send an empty "Start" message to let the privileged side know we
Expand Down
46 changes: 46 additions & 0 deletions js/promise_util.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import { PromiseRejectEvent } from "./libdeno";

/* tslint:disable-next-line:no-any */
const rejectMap = new Map<Promise<any>, string>();
// For uncaught promise rejection errors

/* tslint:disable-next-line:no-any */
const otherErrorMap = new Map<Promise<any>, string>();
// For reject after resolve / resolve after resolve errors

export function promiseRejectHandler(
error: Error | string,
event: PromiseRejectEvent,
/* tslint:disable-next-line:no-any */
promise: Promise<any>
) {
switch (event) {
case "RejectWithNoHandler":
rejectMap.set(promise, (error as Error).stack || "RejectWithNoHandler");
break;
case "HandlerAddedAfterReject":
rejectMap.delete(promise);
break;
default:
// error is string here
otherErrorMap.set(promise, `Promise warning: ${error as string}`);
}
}

// Return true when continue, false to die on uncaught promise reject
export function promiseErrorExaminer(): boolean {
if (otherErrorMap.size > 0) {
for (const msg of otherErrorMap.values()) {
console.log(msg);
}
otherErrorMap.clear();
}
if (rejectMap.size > 0) {
for (const msg of rejectMap.values()) {
console.log(msg);
}
rejectMap.clear();
return false;
}
return true;
}
143 changes: 139 additions & 4 deletions libdeno/binding.cc
Original file line number Diff line number Diff line change
Expand Up @@ -138,15 +138,59 @@ void HandleException(v8::Local<v8::Context> context,
}
}

void ExitOnPromiseRejectCallback(
v8::PromiseRejectMessage promise_reject_message) {
const char* PromiseRejectStr(enum v8::PromiseRejectEvent e) {
switch (e) {
case v8::PromiseRejectEvent::kPromiseRejectWithNoHandler:
return "RejectWithNoHandler";
case v8::PromiseRejectEvent::kPromiseHandlerAddedAfterReject:
return "HandlerAddedAfterReject";
case v8::PromiseRejectEvent::kPromiseResolveAfterResolved:
return "ResolveAfterResolved";
case v8::PromiseRejectEvent::kPromiseRejectAfterResolved:
return "RejectAfterResolved";
}
}

void PromiseRejectCallback(v8::PromiseRejectMessage promise_reject_message) {
auto* isolate = v8::Isolate::GetCurrent();
Deno* d = static_cast<Deno*>(isolate->GetData(0));
DCHECK_EQ(d->isolate, isolate);
v8::HandleScope handle_scope(d->isolate);
auto exception = promise_reject_message.GetValue();
auto context = d->context.Get(d->isolate);
HandleException(context, exception);
auto promise = promise_reject_message.GetPromise();
auto event = promise_reject_message.GetEvent();

v8::Context::Scope context_scope(context);
auto promise_reject_handler = d->promise_reject_handler.Get(isolate);

if (!promise_reject_handler.IsEmpty()) {
v8::Local<v8::Value> args[3];
args[1] = v8_str(PromiseRejectStr(event));
args[2] = promise;
/* error, event, promise */
if (event == v8::PromiseRejectEvent::kPromiseRejectWithNoHandler) {
d->pending_promise_events++;
// exception only valid for kPromiseRejectWithNoHandler
args[0] = exception;
} else if (event ==
v8::PromiseRejectEvent::kPromiseHandlerAddedAfterReject) {
d->pending_promise_events--; // unhandled event cancelled
if (d->pending_promise_events < 0) {
d->pending_promise_events = 0;
}
// Placeholder, not actually used
args[0] = v8_str("Promise handler added");
} else if (event == v8::PromiseRejectEvent::kPromiseResolveAfterResolved) {
d->pending_promise_events++;
args[0] = v8_str("Promise resolved after resolved");
} else if (event == v8::PromiseRejectEvent::kPromiseRejectAfterResolved) {
d->pending_promise_events++;
args[0] = v8_str("Promise rejected after resolved");
}
promise_reject_handler->Call(context->Global(), 3, args);
return;
}
}

void Print(const v8::FunctionCallbackInfo<v8::Value>& args) {
Expand Down Expand Up @@ -279,6 +323,48 @@ void SetGlobalErrorHandler(const v8::FunctionCallbackInfo<v8::Value>& args) {
d->global_error_handler.Reset(isolate, func);
}

// Sets the promise uncaught reject handler
void SetPromiseRejectHandler(const v8::FunctionCallbackInfo<v8::Value>& args) {
v8::Isolate* isolate = args.GetIsolate();
Deno* d = reinterpret_cast<Deno*>(isolate->GetData(0));
DCHECK_EQ(d->isolate, isolate);

v8::HandleScope handle_scope(isolate);

if (!d->promise_reject_handler.IsEmpty()) {
isolate->ThrowException(
v8_str("libdeno.setPromiseRejectHandler already called."));
return;
}

v8::Local<v8::Value> v = args[0];
CHECK(v->IsFunction());
v8::Local<v8::Function> func = v8::Local<v8::Function>::Cast(v);

d->promise_reject_handler.Reset(isolate, func);
}

// Sets the promise uncaught reject handler
void SetPromiseErrorExaminer(const v8::FunctionCallbackInfo<v8::Value>& args) {
v8::Isolate* isolate = args.GetIsolate();
Deno* d = reinterpret_cast<Deno*>(isolate->GetData(0));
DCHECK_EQ(d->isolate, isolate);

v8::HandleScope handle_scope(isolate);

if (!d->promise_error_examiner.IsEmpty()) {
isolate->ThrowException(
v8_str("libdeno.setPromiseErrorExaminer already called."));
return;
}

v8::Local<v8::Value> v = args[0];
CHECK(v->IsFunction());
v8::Local<v8::Function> func = v8::Local<v8::Function>::Cast(v);

d->promise_error_examiner.Reset(isolate, func);
}

bool ExecuteV8StringSource(v8::Local<v8::Context> context,
const char* js_filename,
v8::Local<v8::String> source) {
Expand Down Expand Up @@ -354,6 +440,24 @@ void InitializeContext(v8::Isolate* isolate, v8::Local<v8::Context> context,
set_global_error_handler_val)
.FromJust());

auto set_promise_reject_handler_tmpl =
v8::FunctionTemplate::New(isolate, SetPromiseRejectHandler);
auto set_promise_reject_handler_val =
set_promise_reject_handler_tmpl->GetFunction(context).ToLocalChecked();
CHECK(deno_val
->Set(context, deno::v8_str("setPromiseRejectHandler"),
set_promise_reject_handler_val)
.FromJust());

auto set_promise_error_examiner_tmpl =
v8::FunctionTemplate::New(isolate, SetPromiseErrorExaminer);
auto set_promise_error_examiner_val =
set_promise_error_examiner_tmpl->GetFunction(context).ToLocalChecked();
CHECK(deno_val
->Set(context, deno::v8_str("setPromiseErrorExaminer"),
set_promise_error_examiner_val)
.FromJust());

{
auto source = deno::v8_str(js_source.c_str());
CHECK(
Expand Down Expand Up @@ -389,6 +493,7 @@ void InitializeContext(v8::Isolate* isolate, v8::Local<v8::Context> context,
}

void AddIsolate(Deno* d, v8::Isolate* isolate) {
d->pending_promise_events = 0;
d->next_req_id = 0;
d->isolate = isolate;
// Leaving this code here because it will probably be useful later on, but
Expand All @@ -397,7 +502,7 @@ void AddIsolate(Deno* d, v8::Isolate* isolate) {
// d->isolate->SetAbortOnUncaughtExceptionCallback(AbortOnUncaughtExceptionCallback);
// d->isolate->AddMessageListener(MessageCallback2);
// d->isolate->SetFatalErrorHandler(FatalErrorCallback2);
d->isolate->SetPromiseRejectCallback(deno::ExitOnPromiseRejectCallback);
d->isolate->SetPromiseRejectCallback(deno::PromiseRejectCallback);
d->isolate->SetData(0, d);
}

Expand Down Expand Up @@ -490,6 +595,36 @@ int deno_respond(Deno* d, void* user_data, int32_t req_id, deno_buf buf) {
return 0;
}

void deno_check_promise_errors(Deno* d) {
if (d->pending_promise_events > 0) {
auto* isolate = d->isolate;
v8::Locker locker(isolate);
v8::Isolate::Scope isolate_scope(isolate);
v8::HandleScope handle_scope(isolate);

auto context = d->context.Get(d->isolate);
v8::Context::Scope context_scope(context);

v8::TryCatch try_catch(d->isolate);
auto promise_error_examiner = d->promise_error_examiner.Get(d->isolate);
if (promise_error_examiner.IsEmpty()) {
d->last_exception =
"libdeno.setPromiseErrorExaminer has not been called.";
return;
}
v8::Local<v8::Value> args[0];
auto result = promise_error_examiner->Call(context->Global(), 0, args);
if (try_catch.HasCaught()) {
deno::HandleException(context, try_catch.Exception());
}
d->pending_promise_events = 0; // reset
if (!result->BooleanValue(context).FromJust()) {
// Has uncaught promise reject error, exiting...
exit(1);
}
}
}

void deno_delete(Deno* d) {
d->isolate->Dispose();
delete d;
Expand Down
2 changes: 2 additions & 0 deletions libdeno/deno.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ int deno_execute(Deno* d, void* user_data, const char* js_filename,
// libdeno.recv() callback. Check deno_last_exception() for exception text.
int deno_respond(Deno* d, void* user_data, int32_t req_id, deno_buf buf);

void deno_check_promise_errors(Deno* d);

const char* deno_last_exception(Deno* d);

void deno_terminate_execution(Deno* d);
Expand Down
13 changes: 11 additions & 2 deletions libdeno/internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ struct deno_s {
std::string last_exception;
v8::Persistent<v8::Function> recv;
v8::Persistent<v8::Function> global_error_handler;
v8::Persistent<v8::Function> promise_reject_handler;
v8::Persistent<v8::Function> promise_error_examiner;
int32_t pending_promise_events;
v8::Persistent<v8::Context> context;
v8::Persistent<v8::Map> async_data_map;
deno_recv_cb cb;
Expand All @@ -32,10 +35,16 @@ void Print(const v8::FunctionCallbackInfo<v8::Value>& args);
void Recv(const v8::FunctionCallbackInfo<v8::Value>& args);
void Send(const v8::FunctionCallbackInfo<v8::Value>& args);
void SetGlobalErrorHandler(const v8::FunctionCallbackInfo<v8::Value>& args);
void SetPromiseRejectHandler(const v8::FunctionCallbackInfo<v8::Value>& args);
void SetPromiseErrorExaminer(const v8::FunctionCallbackInfo<v8::Value>& args);
static intptr_t external_references[] = {
reinterpret_cast<intptr_t>(Print), reinterpret_cast<intptr_t>(Recv),
reinterpret_cast<intptr_t>(Print),
reinterpret_cast<intptr_t>(Recv),
reinterpret_cast<intptr_t>(Send),
reinterpret_cast<intptr_t>(SetGlobalErrorHandler), 0};
reinterpret_cast<intptr_t>(SetGlobalErrorHandler),
reinterpret_cast<intptr_t>(SetPromiseRejectHandler),
reinterpret_cast<intptr_t>(SetPromiseErrorExaminer),
0};

Deno* NewFromSnapshot(void* user_data, deno_recv_cb cb);

Expand Down
12 changes: 12 additions & 0 deletions libdeno/libdeno_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -193,3 +193,15 @@ TEST(LibDenoTest, DataBuf) {
EXPECT_EQ(data_buf_copy.data_ptr[1], 8);
deno_delete(d);
}

TEST(LibDenoTest, PromiseRejectCatchHandling) {
static int count = 0;
Deno* d = deno_new([](auto _, int req_id, auto buf, auto data_buf) {
// If no error, nothing should be sent, and count should not increment
count++;
});
EXPECT_TRUE(deno_execute(d, nullptr, "a.js", "PromiseRejectCatchHandling()"));

EXPECT_EQ(count, 0);
deno_delete(d);
}
40 changes: 40 additions & 0 deletions libdeno/libdeno_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,3 +133,43 @@ global.DataBuf = () => {
b[0] = 9;
b[1] = 8;
};

global.PromiseRejectCatchHandling = () => {
let count = 0;
let promiseRef = null;
// When we have an error, libdeno sends something
function assertOrSend(cond) {
if (!cond) {
libdeno.send(new Uint8Array([42]));
}
}
libdeno.setPromiseErrorExaminer(() => {
assertOrSend(count === 2);
});
libdeno.setPromiseRejectHandler((error, event, promise) => {
count++;
if (event === "RejectWithNoHandler") {
assertOrSend(error instanceof Error);
assertOrSend(error.message === "message");
assertOrSend(count === 1);
promiseRef = promise;
} else if (event === "HandlerAddedAfterReject") {
assertOrSend(count === 2);
assertOrSend(promiseRef === promise);
}
// Should never reach 3!
assertOrSend(count !== 3);
});

async function fn() {
throw new Error("message");
}

(async () => {
try {
await fn();
} catch (e) {
assertOrSend(count === 2);
}
})();
}
Loading

0 comments on commit 45d3b89

Please sign in to comment.