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

Idea: Switching Mammoth to use native async/await might yield better stack traces. #205

Open
cakoose opened this issue Jan 12, 2021 · 6 comments

Comments

@cakoose
Copy link
Collaborator

cakoose commented Jan 12, 2021

Historically, JavaScript stack traces get truncated once you yield back to the event loop, e.g. for I/O. In the last few years, V8 added async stack traces, which fix that problem if you use native async/await instead of raw promises. This makes it way easier to track down errors.

It looks like Mammoth uses raw promises, so the stack traces I get are truncated. I think if it used native async/await, the stack traces would be better. I'm not familiar enough with Mammoth to understand the difficulties and downsides of doing that, though.

A workaround: wrap each function with code that catches exceptions and fixes the stack trace: GitHub gist. That works if I'm only using a few functions or there are only a few call sites. But Mammoth has a pretty big API and I use Mammoth everywhere in my app, so that would be too tedious.

@cakoose
Copy link
Collaborator Author

cakoose commented Jan 13, 2021

Small demo of the problem: https://gist.github.com/cakoose/5271c469142205d37c600000cc22bb3e

@martijndeh
Copy link
Contributor

Mammoth automatically executes your query when you await it. This is done by adding a .then() so the whole query looks like a promise and thus can be await'ed.

There isn't a function which we can change to async to make sure native async / await is triggered. How do you think we can fix this?

  1. Create an async exec() { .. } on every function — assuming this will trigger native async / await (is this true?). (a) This would be in addition to .then() (b) remove .then() so you must always explicitly .exec() your queries.
  2. Create some sort of trace capture mechanism similar to something like this one https://knexjs.org/#Installation-asyncStackTraces
  3. Anything else?

@cakoose
Copy link
Collaborator Author

cakoose commented Jul 27, 2021

Re 1: Using an explicit .exec() appears to yield good stack traces: snippet. I would love to get it working for the plain .then as well, but I don't know if it's possible. (I tried asking on the v8-users mailing list but never got a response.)

Re 2: That style of stack trace preservation isn't as efficient, and thus is not recommended in production. Having something only work in dev is still definitely useful, but I'm hoping we can get V8 native async stack traces working, which will work in both.

Tangent: Is an explicit .exec() function a good idea?

Zapatos has an explicit .run(conn). Knex seems to do what Mammoth does. I don't know which is better but here are some thoughts:

When I first started using Mammoth, I preferred the idea of Zapatos' explicit .run(conn), since it separated the concerns of building the query and executing the query. But after having used Mammoth for a while, I've gotten used to the convenience of just doing await 😅 .

There's also a small practical advantage to Mammoth's approach: I can use the ESLint no-floating-promises rule to detect when I forget to await. In theory, we should be able to write an ESLint rule to detect cases where someone builds a query but forgets to run it, but nobody has written that yet.

@martijndeh
Copy link
Contributor

Re 2: That style of stack trace preservation isn't as efficient, and thus is not recommended in production.

I do not see any performance differences in the happy path when enabling asyncStackTraces in Knex (there is a simple benchmark in the repo now). Are you referring to the extra memory to keep track of the stack?

@cakoose
Copy link
Collaborator Author

cakoose commented Aug 10, 2021

I do not see any performance differences in the happy path when enabling asyncStackTraces in Knex (there is a simple benchmark in the repo now). Are you referring to the extra memory to keep track of the stack?

I was just going off of the warning in Knex's docs ("This has small performance overhead, so it is advised to use only for development.") and the general warnings around "async stack trace" libraries in general, e.g. longjohn.

I assume the cost is mainly in generating the stack trace, but it probably adds some GC pressure too But maybe that overhead is not actually that much in this context because a remote DB query is already relatively expensive?

Where's the benchmark? I wonder if it's measuring a case where generating the stack is cheap for whatever reason, e.g. shallow stack.

@martijndeh
Copy link
Contributor

The benchmark is here https://github.com/Ff00ff/mammoth/tree/master/workspaces/sql-benchmarks. You are right, the stack isn't too deep so maybe that's why there is no significant overhead.

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

2 participants