The dreaded "throwing after await" #34
Comments
|
N.B: turning const binomial = async () => await cherrypie();into async function binomial () { await cherrypie(); }appears to make no difference whatsoever. |
|
@fasterthanlime Thanks, that does look suspicious. I will look at it next week after I have handed in my master thesis. In the meantime, I recommend upgrading to node v8.1.x where we landed some changes to how promises show up in |
|
Cool, thanks! Here's the output with node v8.1.2: |
|
Thanks, I didn't really expect the upgrade to fix it. It looks like there is another bug, |
|
Happy to help! I have a whole suite of tests like that, because I was tired of getting useless stack traces in my error reports: https://github.com/itchio/itch/blob/cead3ba6bcbe5c46f57dc6b95b01763e73a87420/src/confidence/stacktrace.spec.ts My current solution is using babel to transpile all Promise code to bluebird, and enable bluebird's longStackTraces collection. Which works well enough, but I feel like native v8 promises + async_hooks should be faster, since bluebird Promises are coroutine-based. There's literally 0 time pressure on this though - before I can switch to native promises + async_hooks, electron has to release with node v8.0.0, and that'll probably take a few weeks! |
|
Your "Throwing first thing" creates 13 Promise instances, I'm having some trouble identifying them all. Could you demonstrate the issue where edit: I have pushed a new master that removes some of the |
Interesting, the issue doesn't seem to appear with pure promises |
|
@fasterthanlime Well I'm no expert in async/await, but I'm pretty sure you can't just remove the
|
That's what I did though :) const cherrypie = () =>
new Promise((resolve, reject) => setTimeout(resolve, 200))
.then(() => {
throw new Error(`after timeout`);
})These all behave the same: // relay the original promise
const v1 = () => functionReturningPromise();
// wrap the original promise - if it resolves the wrapper
// will resolve, if it rejects the wrapper will reject
const v2 = () => Promise.resolve(functionReturningPromise());
// async functions "rethrow" rejections and can "return"
// resolved values asynchronously
const v3 = async () => await functionReturningPromise();
// don't write that ;)
const v4 = () => {
return new Promise((resolve, reject) => {
functionReturningPromise().then(resolve).catch(reject);
});
}edit: I should clarify - according to my understanding of async/await and promises, those should all: resolve or reject according to whatever the Promise returned by The noise bothers me a lot less than missing frames :) |
They have subtle differences in the standard too. It is about how many rounds the microtask queue take.
I know, but that was an issue I understood :) |
|
Okay, I have reduced it to promises. It is not exactly the same but the issue is the same: throwing first thing: 'use strict';
function binomial() {
return new Promise(function binomialPromise() {
throw new Error('throwing first thing');
});
}
function abacus() {
return Promise.resolve().then(function abacusThen() {
return binomial();
});
}
abacus().catch(function (e) {
process._rawDebug(e);
});after timeout: 'use strict';
function binomial() {
return Promise.resolve(/* timeout */).then(function binomialThen() {
throw new Error('after timeout');
});
}
function abacus() {
return Promise.resolve().then(function abacusThen() {
return binomial();
});
}
abacus().catch(function (e) {
process._rawDebug(e);
}); |
|
I have debugged it some more. The stack trace is actually correct, the problem is that v8 calls I will think about possible solutions to this, but I'm not sure it can be fixed. throwing first thing async function binomial() {
const a = new Error('throwing first thing'); a.stack; throw a;
}
async function abacus() {
await binomial();
}
async function main() {
try {
await abacus();
} catch (e) {
process._rawDebug(e.stack);
}
}
main();after timeout async function binomial() {
await Promise.resolve(/* timeout */);
const a = new Error('after timeout'); a.stack; throw a;
}
async function abacus() {
await binomial();
}
async function main() {
try {
await abacus();
} catch (e) {
process._rawDebug(e.stack);
}
}
main(); |
|
Ah, so the only way out would be to have a new hook in node when it creates the Exception (rather than when |
Yep. I tried overwriting The primary reason for lazy creation of |
|
I've tried following the discussion in nodejs/node but it flew way over my head :) Is there any workaround that would work with node v8.2.1? Or is it too early still? No pressure at all btw, just checking in as I'm still very interested in this. |
|
@fasterthanlime I'm not sure what discussion that is. There is a somewhat interesting discussion here: nodejs/node#11865 Nothing sufficiently new has happened in v8.2.1. I suppose it might be possible to setup node errors and users error to not lazy-build the stack, but even so, native JavaScript are still going to lazy-build the stack. I think the only way to really solve this, is to involve the v8 team. I asked in nodejs/node#11865 if we can get a |
|
I think I have a solution for this in my follow-promise-resolve branch. I use the It passes the existing tests as well as my new ones, however it needs a little more cleanup before I raise a PR. |
|
@jbunton-atlassian I'm excited that you are investigating this, but I'm very skeptical. I'm very skeptical because no matter what you won't get the frames from the point of view where function asyncFunction(callback) {
process.nextTick(callback, new Error('error'))
}
asyncFunction(function (err) {
process.nextTick(() => throw err)
})will suffer from the same issue, because the long-stack-trace is collected at
Using the
As I see it, you just represent the long-stack-trace as a linked list. So I don't really see what extra information you get. Can you clarify?
Okay, I will be happy to review when you are done. |
|
Hi there. I think I've solved this in #40. I'd love any feedback or testing anybody could give this code :) You can try it out by installing it from my repo: |
This might be completely expected, I'm not completely up-to-date on the state of node.js, but:
Repro steps
Versions
Output
Expected output
In the second trace, something like:
The text was updated successfully, but these errors were encountered: