Handle promise reject handler added later issue #959

kevinkassimo · 2018-10-11T05:52:10Z

Closes #936
Rationale see #936 comments (handler implementation moved to TS side).

v8 emits kPromiseHandlerAddedAfterReject when a catch block is found in async function, that cancels a previous kPromiseRejectWithNoHandler. Postpone error handling and exit to the end of each event loop cycle. tests/async_error.ts.out actually now matches that running under Node. ("world" is printed first: error handling only happens after script running to its end)

An alternative C++ only implementation could be found here

ry

Great! I'm not sure about the API... but glad these tricky bugs are being nailed down.

@piscisaureus Please review.

ry · 2018-10-11T06:10:40Z

libdeno/deno.h

@@ -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_reject_events(Deno* d);


Please add tests to libdeno/libdeno_test.cc

kevinkassimo · 2018-10-11T16:28:12Z

libdeno/binding.cc

+    }
+    d->pending_promise_events = 0;  // reset
+    if (result->Uint32Value(context).FromJust() == 1) {
+      exit(1);


Checking returned value and exit here is not quite ideal... However, if I were to use os.exit(...) from TS side in promiseErrorExaminer it would trigger segfault.

Not quite sure what I did wrong

ry · 2018-10-11T16:54:14Z

libdeno/binding.cc

@@ -322,6 +396,43 @@ bool Execute(v8::Local<v8::Context> context, const char* js_filename,
  return ExecuteV8StringSource(context, js_filename, source);
 }

+v8::Local<v8::Object> InitializeConstants(v8::Isolate* isolate,
+                                          v8::Local<v8::Context> context) {
+  auto constants_val = v8::Object::New(isolate);


Put these directly into the libdeno namespace, not libdeno.constants

ry · 2018-10-11T16:55:24Z

libdeno/binding.cc

+
+  auto promise_reject_event_val = v8::Object::New(isolate);
+  CHECK(promise_reject_event_val
+            ->Set(context, deno::v8_str("kPromiseRejectWithNoHandler"),


put this directly into the libdeno namespace, not libdeno.constants.promiseRejectEvents

ry · 2018-10-11T16:59:20Z

libdeno/libdeno_test.js

+      assertOrSend(count === 2);
+    }
+  })();
+}


ry · 2018-10-11T17:02:10Z

js/libdeno.ts

+  setPromiseRejectHandler: (
+    handler: (
+      error: Error,
+      event: number,


How about about

event: "RejectWithNoHandler" | "HandlerAddedAfterReject" | "ResolveAfterResolved" | "RejectAfterResolved"

Event types from v8 here are integers. Not quite sure if we want to covert to string reprs

The only concern would be about performance.. presumably numbers are faster.
Is this hot code?

The handler is invoked twice for each caught promise reject. Not quite sure if it is considered hot enough

I think mapping the V8 numbers to strings in C++ isn't so bad. This will simply the libdeno JS interface quite a bit.

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

Thanks! Added the function.

ry · 2018-10-11T17:05:50Z

js/promise_util.ts

+const promiseRejectEvents = libdeno.constants.promiseRejectEvents;
+
+/* tslint:disable-next-line:no-any */
+const promiseRejectMap = new Map<Promise<any>, string>();


:%s/promiseRejectMap/rejectMap/g

ry · 2018-10-11T17:06:08Z

js/promise_util.ts

+/* tslint:disable-next-line:no-any */
+const promiseRejectMap = new Map<Promise<any>, string>();
+/* tslint:disable-next-line:no-any */
+const otherPromiseErrorMap = new Map<Promise<any>, string>();


%s/otherPromiseErrorMap/otherErrorMap/g

ry · 2018-10-11T17:09:28Z

js/promise_util.ts

+    promiseRejectMap.clear();
+    return 1;
+  }
+  return 0;


deno.exit() requires message passing - which probably doesn't work in this state of error.

maybe we should add libdeno.exit() as a way to quickly exit.. but i think your solution here is fine. maybe make the return value boolean.

ry

LGTM, but I'll wait for @piscisaureus to review.

too rubberstamp happy today

kevinkassimo added 2 commits October 10, 2018 22:45

Handle promise handler added later issue

4e24b79

Add async tests from issue

2b3a928

kevinkassimo changed the title ~~Handle promise handler added later issue~~ Handle promise reject handler added later issue Oct 11, 2018

ry reviewed Oct 11, 2018

View reviewed changes

Add libdeno test

88a22c0

kevinkassimo force-pushed the error/async_js branch from 93a9ee1 to 88a22c0 Compare October 11, 2018 16:07

Rename

0d6d612

kevinkassimo commented Oct 11, 2018

View reviewed changes

ry reviewed Oct 11, 2018

View reviewed changes

libdeno/libdeno_test.js

assertOrSend(count === 2);

}

})();

}

Copy link

Member

ry Oct 11, 2018

Choose a reason for hiding this comment

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

nice work!

ry reviewed Oct 11, 2018

View reviewed changes

kevinkassimo added 2 commits October 11, 2018 11:43

Rename; spill constants directly on libdeno; change return type

25365ef

Use string for promise event (less libdeno binding)

907efd6

ry requested a review from piscisaureus October 11, 2018 21:53

ry reviewed Oct 11, 2018

View reviewed changes

piscisaureus self-assigned this Oct 12, 2018

piscisaureus previously approved these changes Oct 12, 2018

View reviewed changes

ry approved these changes Oct 12, 2018

View reviewed changes

ry merged commit 45d3b89 into denoland:master Oct 12, 2018

kevinkassimo deleted the error/async_js branch December 27, 2019 07:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle promise reject handler added later issue #959

Handle promise reject handler added later issue #959

kevinkassimo commented Oct 11, 2018

ry left a comment

ry Oct 11, 2018

kevinkassimo Oct 11, 2018

ry Oct 11, 2018

ry Oct 11, 2018

ry Oct 11, 2018

ry Oct 11, 2018

kevinkassimo Oct 11, 2018

ry Oct 11, 2018

kevinkassimo Oct 11, 2018 •

edited

Loading

ry Oct 11, 2018

ry Oct 11, 2018 •

edited

Loading

kevinkassimo Oct 11, 2018

ry Oct 11, 2018

ry Oct 11, 2018

ry Oct 11, 2018

ry left a comment •

edited

Loading

Handle promise reject handler added later issue #959

Handle promise reject handler added later issue #959

Conversation

kevinkassimo commented Oct 11, 2018

ry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkassimo Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry left a comment • edited Loading

Choose a reason for hiding this comment

kevinkassimo Oct 11, 2018 •

edited

Loading

ry Oct 11, 2018 •

edited

Loading

ry left a comment •

edited

Loading