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

Improve error stacks for async ops #2820

Merged
merged 4 commits into from Aug 27, 2019

Conversation

@nayeemrmn
Copy link
Contributor

commented Aug 26, 2019

This moves error construction out of the message handler so that the call stack reaches user code. Also refactors a bit.

test.js:

/* Print a synchronous op error. */
try {
  Deno.openSync("nonexistent.txt");
} catch (e) {
  console.log(e.stack);
}

/* Print an asynchronous op error. */
(async () => {
  await Deno.open("nonexistent.txt").catch(e => {
    console.log(e.stack);
  });
})();

Before:

NotFound: No such file or directory (os error 2)
    at toDenoError (js/dispatch_json.ts:39:10)
    at sendSync (js/dispatch_json.ts:72:11)
    at Object.openSync (js/files.ts:26:15)
    at file:///mnt/c/Users/Nayeem/projects/deno/test.js:3:8
NotFound: No such file or directory (os error 2)
    at toDenoError (js/dispatch_json.ts:39:10)
    at asyncMsgFromRust$1 (js/dispatch_json.ts:51:20)
    at asyncMsgFromRust$2 (js/dispatch.ts:99:7)
    at handleAsyncMsgFromRust (shared_queue.js:171:9)

After:

NotFound: No such file or directory (os error 2)
    at unwrapResponse (js/dispatch_json.ts:40:11)
    at sendSync (js/dispatch_json.ts:67:10)
    at Object.openSync (js/files.ts:26:15)
    at file:///mnt/c/Users/Nayeem/projects/deno/test.js:3:8
NotFound: No such file or directory (os error 2)
    at unwrapResponse (js/dispatch_json.ts:40:11)
    at sendAsync (js/dispatch_json.ts:85:10)
    at async Object.open (js/files.ts:40:15)
    at async file:///mnt/c/Users/Nayeem/projects/deno/test.js:10:3

Fixes #2692. IMO, the biggest issue is the stack. Otherwise Process::status() rejecting with BadResource is sufficient, as long as the user knows that's what's happening.

Also see #2703. The stack trace shown on abort doesn't include any async frames, which you'd see in the above example if I let the error go uncaught.

@ry

This comment has been minimized.

Copy link
Collaborator

commented Aug 27, 2019

Nice!

You inadvertently left some third_party changes in there - run git submodule update

Can you add a test that shows the desired stack as simply as possible? Ideally in the unit tests.

@nayeemrmn nayeemrmn force-pushed the nayeemrmn:error-stacks branch 4 times, most recently from 019bce2 to a7aee29 Aug 27, 2019

@nayeemrmn nayeemrmn force-pushed the nayeemrmn:error-stacks branch from a7aee29 to b9e0796 Aug 27, 2019

js/dispatch_json.ts Show resolved Hide resolved
js/dispatch_json.ts Show resolved Hide resolved
@ry
ry approved these changes Aug 27, 2019
Copy link
Collaborator

left a comment

LGTM

@ry ry merged commit b6a4ec7 into denoland:master Aug 27, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@nayeemrmn nayeemrmn deleted the nayeemrmn:error-stacks branch Aug 27, 2019

@bartlomieju bartlomieju referenced this pull request Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.