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

Async stack traces for callback errors #1077

Closed
aalexgabi opened this issue Feb 19, 2020 · 23 comments
Closed

Async stack traces for callback errors #1077

aalexgabi opened this issue Feb 19, 2020 · 23 comments
Labels
documentation The issue will improve the docs ✭ help wanted ✭

Comments

@aalexgabi
Copy link

What problem are you trying to solve?

I need to know the caller that generated a given http error.

Describe the feature

I use node 13 which has async stack traces but I don't have the caller stack trace when using got. Instead I get this stack:

    err HTTPError: Response code 404 (Not Found)
        at EventEmitter.<anonymous> (.../node_modules/got/dist/
source/as-promise.js:118:31)
        at processTicksAndRejections (internal/process/task_queues.js:97:5) {
      name: 'HTTPError'
    }

I guess this happens because the connection can be used by multiple callers but there should be a way to have the whole stack.

@szmarczak
Copy link
Collaborator

This works as expected: EventEmitter.<anonymous> is the handler for the response event received from the native Node.js call.

@szmarczak
Copy link
Collaborator

Related: #795

@szmarczak szmarczak added the question The issue is a question about Got label Feb 19, 2020
@szmarczak
Copy link
Collaborator

I don't think we can do much about this. This is just how Node.js works. One possible solution would be to create a stacktrace before making the request and create another one when erroring and then merge these two.

@sindresorhus What do you think?

@szmarczak szmarczak changed the title Async stack traces for errors Async stack traces for callback errors Feb 19, 2020
@sindresorhus
Copy link
Owner

@szmarczak
Copy link
Collaborator

We could, but:

  1. the API is marked experimental so it will give a warning (it's live since Node.js 8.1) - but I think it's stable for this use case as I don't see any issues that would break this
  2. the change would be visible globally.

I think we should create another .md file and show how to achieve the desired result step-by-step.

@aalexgabi
Copy link
Author

One possible solution would be to create a stacktrace before making the request and create another one when erroring and then merge these two.

@szmarczak This approach works. Our team used in many projects that involved multiplexing a connection or a event emitter. The only think I would worry about is the CPU load associated with generating the stack.

Maybe stack trace generation performance has improved in the last years. I have seen some articles talking about zero cost async stack traces but I'm not sure if it applies:

@szmarczak
Copy link
Collaborator

@aalexgabi The zero-cost stacktraces you mentioned are related to async/await (and are already on by default), not I/O operations.

@sindresorhus
Copy link
Owner

the change would be visible globally.

Yeah, we cannot modify anything globally.

But would it have to be global? I was thinking of storing the stack in the async context and then merge it on error.

I think we should create another .md file and show how to achieve the desired result step-by-step.

👍

@szmarczak
Copy link
Collaborator

But would it have to be global? I was thinking of storing the stack in the async context and then merge it on error.

That would work too, but the question is how much performance you would have to give up, since on every async task there would be called Error.captureStackTrace...

@sindresorhus
Copy link
Owner

Maybe we should just make an independent module that creates long stack traces using async hooks contexts and then recommend that, but warn that it might have a performance impact. Then the user can decide whether it's worth it or not.

@szmarczak
Copy link
Collaborator

szmarczak commented Mar 5, 2020

async_hooks is now [almost] stable >=13.10.0 https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V13.md/#notable-changes-1

This was referenced Mar 10, 2020
@szmarczak szmarczak added documentation The issue will improve the docs ✭ help wanted ✭ and removed question The issue is a question about Got labels Apr 1, 2020
@PSeitz
Copy link

PSeitz commented Sep 11, 2020

Would the --async-stack-traces option in nodejs12 solve this issue?
http://thecodebarbarian.com/async-stack-traces-in-node-js-12.html

@szmarczak
Copy link
Collaborator

@PSeitz It's already on by default IIRC.

@szmarczak szmarczak mentioned this issue Jun 5, 2021
2 tasks
@rightaway
Copy link

@szmarczak Is there any workaround for this even if it makes performance worse? Trying to debug some issues and the stack traces of where the request was initially made from would really help.

@szmarczak
Copy link
Collaborator

You can capture the stack trace in a handler and store it inside options.context.stack.

@szmarczak
Copy link
Collaborator

import got from 'got';

const instance = got.extend({
	handlers: [
		(options, next) => {
			Error.captureStackTrace(options.context);

			return next(options);
		}
	],
	hooks: {
		beforeError: [
			error => {
				error.source = error.options.context.stack.split('\n');

				return error;
			}
		]
	}
});

try {
	await instance('https://httpbin.org/status/400');
} catch (error) {
	console.error(error);
}

@szmarczak
Copy link
Collaborator

https://github.com/sindresorhus/got/blob/main/documentation/async-stack-traces.md#conclusion

@jgehrcke
Copy link

@szmarczak thank you for crafting this document!

@bencmbrook
Copy link

bencmbrook commented May 20, 2022

@szmarczak I'm using that snippet but getting "Cannot add property stack, object is not extensible" when setting stack on options.context via Error.captureStackTrace(options.context);. Does this have an Object.freeze / .preventExtensions / .seal now?

@szmarczak
Copy link
Collaborator

You're right, indeed. https://github.com/sindresorhus/got/blame/main/source/core/options.ts#L2475

This should be removed when #1459 was merged. I just ran the example from the docs and for some reason it worked, but that's weird. Can you send me a repro?

@szmarczak szmarczak reopened this May 20, 2022
@bencmbrook
Copy link

I can't send a full repro at the moment, but I think it's likely you're not in strict mode ('use strict';) if you're not getting the TypeError

FWIW, I worked around this with

const stackTraceHandler = (options, next) => {
  const context = {};
  Error.captureStackTrace(context, stackTraceHandler);
  options.context = { ...options.context, stack: context.stack };
  return next(options);
};

...

got.extend({
  handlers: [stackTraceHandler],
  // ...
});

@bencmbrook
Copy link

Also in case it's helpful to any future readers using TypeScript, here's my working TypeScript version

import got, { BeforeErrorHook, HandlerFunction, RequestError } from 'got';

const stackTraceHandler: HandlerFunction = (options, next) => {
  const context: { stack?: string } = {};
  Error.captureStackTrace(context, stackTraceHandler);
  options.context = { ...options.context, stack: context.stack };
  return next(options);
};

const addSourceStackTraceToError: BeforeErrorHook = (error: RequestError) => {
  error.stack = `${error.stack}\n---Source Stack---\n${error.options.context.stack}`;
  return error;
};

export default got.extend({
  handlers: [stackTraceHandler],
  hooks: {
    beforeError: [addSourceStackTraceToError],
  },
});

Adding the source property to the error object was causing issues so I just decided to append to the existing stack.

@hvrauhal
Copy link

hvrauhal commented Aug 8, 2023

It would be great if the above typescript version was included in the document https://github.com/sindresorhus/got/blob/main/documentation/async-stack-traces.md#conclusion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation The issue will improve the docs ✭ help wanted ✭
Projects
None yet
Development

No branches or pull requests

8 participants