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

Preserve stacktrace when wrapping errors #935

Merged
merged 4 commits into from
Nov 23, 2019

Conversation

szmarczak
Copy link
Collaborator

Fixes #795

Checklist

  • I have read the documentation.
  • I have included a pull request description of my changes.
  • I have included some tests.
  • If it's a new feature, I have included documentation updates.

t.true(error.stack.includes('at Object.request'));

// The first `at get` points to where the error was wrapped,
// the second `at get` points to the real cause.
Copy link
Collaborator Author

@szmarczak szmarczak Nov 17, 2019

Choose a reason for hiding this comment

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

Now:

GotError: foobar
    at get (/home/szm/Desktop/got/source/request-as-event-emitter.ts:250:15)
+   at Object.request (/home/szm/Desktop/got/test/error.ts:206:10)
+   at get (/home/szm/Desktop/got/source/request-as-event-emitter.ts:248:31)
    at Immediate.<anonymous> (/home/szm/Desktop/got/source/request-as-event-emitter.ts:313:4)

Before:

GotError: foobar
    at get (/home/szm/Desktop/got/source/request-as-event-emitter.ts:250:15)
    -- the line above only points to where the error was captured  --
	-- missing what caused the error --
    at Immediate.<anonymous> (/home/szm/Desktop/got/source/request-as-event-emitter.ts:313:4)

@szmarczak
Copy link
Collaborator Author

szmarczak commented Nov 17, 2019

As you can see, the stacktrace begins with at Immediate.<anonymous>. It's this. But what called it, we don't know. See #447 (comment)

To fix this, we need to either remove setImmediate (no big impact on performance) or capture another stacktrace there (this would have a big impact on performance). Or... leave this as is, because it's default Node behavior.

@sindresorhus
Copy link
Owner

To fix this, we need to either remove setImmediate (no big impact on performance)

Why do we even have it there? From what I faintly can remember, it had something to do with a race issue. Maybe we could fix it another way and remove the setImmediate?

capture another stacktrace there (this would have a big impact on performance)

I doubt it has big impact on performance relatively to networking.


Did you check if the setImmediate problem is the same on Node.js 13 too? Later Node.js versions got much better support for preserving async stack traces.

@szmarczak
Copy link
Collaborator Author

Did you check if the setImmediate problem is the same on Node.js 13 too? Later Node.js versions got much better support for preserving async stack traces.

Yep, it's the same.

I doubt it has big impact on performance relatively to networking.

(() => {
    let timings = [];
    let start;
    for (let i = 0; i < 100000; i++) {
        start = performance.now();
        (new Error()).stack;
        timings.push(performance.now() - start);
    }

	return timings.reduce((a, b) => a + b) / timings.length;
})();

gives me 0.0075 ms / stacktrace. So you're right. I thought it would be time-consuming.

From what I faintly can remember, it had something to do with a race issue. Maybe we could fix it another way and remove the setImmediate?

It was introduced here: #304

We can remove it because the errors are handled in a much different way than at the time.

@szmarczak
Copy link
Collaborator Author

I'll fix this in a few hours, need sleep now

@szmarczak
Copy link
Collaborator Author

We need setImmediate to allow calling promise.json() etc. Promises are executed immediately, so if there were no setImmediate, promise.json() would have no effect on the headers.

@sindresorhus sindresorhus changed the title Recover stacktrace when wrapping errors Preserve stacktrace when wrapping errors Nov 23, 2019
@sindresorhus sindresorhus merged commit 8874a45 into sindresorhus:master Nov 23, 2019
@magnusottosson
Copy link

@szmarczak Is this supposed to solve the async stack trace problems? On 10.6.0 I get a stack trace that look like this:

HTTPError: Response code 400 (Bad Request) at EventEmitter.<anonymous> ([redacted]/node_modules/got/dist/source/as-promise.js:118:31) at processTicksAndRejections (internal/process/task_queues.js:97:5)

I have no idea where this error occurs :)

My request looks something like this:

const response = await got('someurl', { json: {...}, headers: { 'Cache-Control': 'no-cache, no-store', }, method: 'POST', });

@szmarczak
Copy link
Collaborator Author

@magnusottosson The title of the PR says "when wrapping errors", not "when handling async errors". You're confusing this PR with #1077

@magnusottosson
Copy link

Ah sorry. My bad :)

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

Successfully merging this pull request may close these issues.

Wrapping RequestError obscures its source
3 participants