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.

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

Unable to catch error thrown inside async function #936

Closed
qoh opened this issue Oct 7, 2018 · 4 comments
Closed

Unable to catch error thrown inside async function #936

qoh opened this issue Oct 7, 2018 · 4 comments

Comments

@qoh
Copy link
Contributor

qoh commented Oct 7, 2018

async function fn() {
	throw new Error("message");
}
async function call() {
	try {
		console.log("before await fn()");
		await fn();
		console.log("after await fn()");
	} catch (error) {
		console.log("catch");
	}
	console.log("after try-catch");
}
call().catch(error => console.log("outer catch"));
$ deno async-fn.ts
Compiling /.../async-fn.ts
before await fn()
Error: message
    at fn (file:///.../async-fn.ts:2:8)
    at call (file:///.../async-fn.ts:7:9)
    at eval (file:///.../async-fn.ts:14:1)
    at DenoCompiler.eval [as _globalEval] (<anonymous>)
    at DenoCompiler._gatherDependencies (deno/js/compiler.ts:214:10)
    at DenoCompiler.run (deno/js/compiler.ts:560:12)
    at denoMain (deno/js/main.ts:72:12)
    at deno_main.js:1:1

If fn is synchronous instead, it works fine:

- async function fn() {
+ function fn() {
$ deno sync-fn.ts
Compiling /.../sync-fn.ts
before await fn()
catch
after try-catch

I thought it might be related to #919, but this also happens with my own build of master after that was closed.

@kevinkassimo
Copy link
Contributor

kevinkassimo commented Oct 8, 2018

I think it is probably related to the following code:

deno/libdeno/binding.cc

Lines 394 to 400 in ffb41e6

// Leaving this code here because it will probably be useful later on, but
// disabling it now as I haven't got tests for the desired behavior.
// d->isolate->SetCaptureStackTraceForUncaughtExceptions(true);
// d->isolate->SetAbortOnUncaughtExceptionCallback(AbortOnUncaughtExceptionCallback);
// d->isolate->AddMessageListener(MessageCallback2);
// d->isolate->SetFatalErrorHandler(FatalErrorCallback2);
d->isolate->SetPromiseRejectCallback(deno::ExitOnPromiseRejectCallback);

From the doc, it seems that the handler could also fire another event saying that a handler is later added

This is probably a related Node issue: nodejs/node#1912
(I might be wrong, not familiar with v8)

@kevinkassimo
Copy link
Contributor

kevinkassimo commented Oct 8, 2018

Okay so I think this is indeed the problem.
I did the following locally in binding.cc

void ExitOnPromiseRejectCallback(
    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);
+ printf("Promise reject try_catch caught, with %d\n", promise_reject_message.GetEvent());
- HandleException(context, exception);
}

The following is printed when running the provided code:

before await fn()
Promise reject try_catch caught, with 0
Promise reject try_catch caught, with 1
catch
after try-catch

From the doc, 0 should correspond to kPromiseRejectWithNoHandler and 1 corresponds to kPromiseHandlerAddedAfterReject. So for some reason V8 first complains about promise rejection, but then decides that actually it is already handled...

Probably this is one the reasons why Node choose not to exit on unhandled promise rejection?
@ry

@kevinkassimo
Copy link
Contributor

@ry I created an experimental branch to play around with this issue (I am not familiar with v8 so would definitely make some mistakes).
However, my attempt seems somehow fixed the issue (now with the intended output), but is also causing a test case to fail:

console.log("hello");
const foo = async () => {
console.log("before error");
throw Error("error");
};
foo();
console.log("world");

With my attempted fix, the output changes from

hello
before error
Error: error
    at foo ([WILDCARD]tests/async_error.ts:4:9)
    at eval ([WILDCARD]tests/async_error.ts:7:1)
    at DenoCompiler.eval [as _globalEval] (<anonymous>)
    at DenoCompiler._gatherDependencies (deno/js/compiler.ts:[WILDCARD])
    at DenoCompiler.run (deno/js/compiler.ts:[WILDCARD])
    at denoMain (deno/js/main.ts:[WILDCARD])
    at deno_main.js:1:1

to

hello
before error
world
Error: error
    at foo ([WILDCARD]tests/async_error.ts:4:9)
    at eval ([WILDCARD]tests/async_error.ts:7:1)
    at DenoCompiler.eval [as _globalEval] (<anonymous>)
    at DenoCompiler._gatherDependencies (deno/js/compiler.ts:[WILDCARD])
    at DenoCompiler.run (deno/js/compiler.ts:[WILDCARD])
    at denoMain (deno/js/main.ts:[WILDCARD])
    at deno_main.js:1:1

which actually matches the output if running under Node. All other tests passes as usual though.

Would like to know comments before submitting a possible PR.

@ztplz
Copy link
Contributor

ztplz commented Oct 8, 2018

@kevinkassimo Yes, I think this is the reason why the test is not normal.

@ry ry closed this as completed in #959 Oct 12, 2018
ry pushed a commit that referenced this issue Oct 12, 2018
ry added a commit to ry/deno that referenced this issue Oct 12, 2018
- Fix promise reject issue (denoland#936)
- Add --types command line flag.
- Add metrics()
- Add redirect follow feature denoland#934
- Fix clearTimer bug denoland#942
- Improve error printing denoland#935
- Expose I/O interfaces Closer, Seeker, ReaderCloser, WriteCloser,
  ReadSeeker, WriteSeeker, ReadWriteCloser, ReadWriteSeeker
- Fix silent death on double await denoland#919
- Add Conn.closeRead() and Conn.closeWrite() denoland#903
ry added a commit that referenced this issue Oct 12, 2018
- Fix promise reject issue (#936)
- Add --types command line flag.
- Add metrics()
- Add redirect follow feature #934
- Fix clearTimer bug #942
- Improve error printing #935
- Expose I/O interfaces Closer, Seeker, ReaderCloser, WriteCloser,
  ReadSeeker, WriteSeeker, ReadWriteCloser, ReadWriteSeeker
- Fix silent death on double await #919
- Add Conn.closeRead() and Conn.closeWrite() #903
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants