Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

Action Items #26

Closed
benjamingr opened this issue Jun 1, 2018 · 15 comments
Closed

Action Items #26

benjamingr opened this issue Jun 1, 2018 · 15 comments

Comments

@benjamingr
Copy link
Member

benjamingr commented Jun 1, 2018

  • Promises need async stack traces in production.
    • V8 team to prototype @MayaLekova @bmeurer
    • We feel that this should work in both development and production similarly.
  • Discuss spec change for not having to wait for 3 ticks for resolving @littledan @MayaLekova - effectively we want to guarantee that at least 1 tick happens and that you can't observe resolution synchronously.
    • Figure out how to propose actual spec change.
    • Gather user data about expectations related to waiting 3 ticks.
    • Poll users who have been transpiling (which does not have this behaviour).
    • Note: this is jQuery's "fault": https://gist.github.com/domenic/3889970
  • Hooks or spec change for throws in the new Promise constructor - where either we provide a hook for multiple resolutions users can use or to change the behavior of the promise constructor to only resolve after the constructor has run (still synchronously, but any throws take precedence).
    https://github.com/nodejs/promise-use-cases/pull/21/files - cc @getify
  • Hooks that enable detection of multiple resolution of promises in the new Promise constructor
  • Hooks for when an await happens so libraries can/might be able to hook with it. (Would be cool if the hook would allow some form of interception?)
  • Expose %RunMicrotasks under a --dangerous-only-for-experimentation-this-will-burn-your-house-asynchronously. Experiment with how people are using it and based on that propose further action items.
  • Node.js should have a better unhandled rejection story
    • @benjamingr to buy @BridgeAR a beer 🍻
    • Landing the PR and discussing the semverness cycle
    • Iron out all the semantics (video call or in person)
  • Document how promises work in Node.js
  • Priority 3: explore the possibility of exposing a hook for "scoping" an async function in order to deal with #expectations-4
  • Promises guide and common patterns as a Node.js guide.
  • Postmortem follow up discussion @benjamingr @yunong @mhdawson
    • Would love to get you on board getting this working to your satisfaction and have you provide criteria for this working in an acceptable way given the constraints promises already impose @misterdjules
@hashseed
Copy link
Member

hashseed commented Jun 1, 2018

Brain dump to explore on async stack traces in production.

Stack traces are expensive because of

  1. having to enter C++ to collect the stack trace
  2. computing inlined frames in optimized code from deopt data, i.e. computing from (optimized code x code offset) to a list of (bytecode x bytecode offset)
  3. converting code offset into source positions, i.e. (bytecode x bytecode offset) -> (script, source position)

The latter two can maybe solved by caching the (code x code offset) -> list of (script x source position) mapping. We already perform caching of StackFrameInfo objects for async stack traces for inspector, but not for optimized frames.

Both can also be made cheaper by performing lazily. (3) is fairly straightforward to perform lazily. (2) is a bit more complicated because the deopt info is thrown away when we deoptimize the optimized code.

We could also explore collecting the stack trace (1) in generated code.

@MayaLekova
Copy link

Pointers for doing the spec change:

  • Create a slide deck
  • Create own repo with a README.md and Issues section for the proposal
  • Hype about it in the IRC channel #tc39
  • Involve a champion from the commitee
    Relevant repos

@misterdjules
Copy link

@benjamingr

Would love to get you on board getting this working to your satisfaction and have you provide criteria for this working in an acceptable way given the constraints promises already impose

Thanks for the mention Benjamin, really appreciate it! How would you like to start/continue the conversation?

@benjamingr
Copy link
Member Author

Thanks for the mention Benjamin, really appreciate it! How would you like to start/continue the conversation?

Well, I think we should set up a call with interested parties to write down where we currently are and where we want to get - as well as what changed since the last time.

  • We're discussing adding an async_hook for throws in the promise constructor which is something I recall you asking for a few years ago. This will allow you to kill the process on throws.
  • We're discussing adding the option to abort the process immediately when an unhandled rejection occurs (as a environment variable configuration). This would still not exit the process immediately but it would exit it before any other microtasks execute. (On an unrelated note - talking to ry this is going to be deno's default behavior which should/may give us some information on how it affects usage).
  • Promises are getting more buffs in development tooling and performance.
  • Node.js is going to kill the process by default on unhandled rejections (detected with GC)

Given the fact promises happened, I would like to get a better understanding of what we can do to give Node.js users a better post-mortem story. I think a major limitation at the moment is that most of the core people working on promises don't have experience working with post-mortem tools and the community has generally not been asking for this (as a whole).

I wonder if a CoW fork for core dump when a rejection happens synchronously + the 'don't wait for more microtick' semantics would be the best we can do.

Also interested if @uzon has any conclusions from their work on the topic from nodejs/post-mortem#45 . @littledan also has an interesting idea in nodejs/post-mortem#45 (comment)

@getify
Copy link
Contributor

getify commented Jun 6, 2018

We're discussing adding an async_hook for throws in the promise constructor which is something I recall you asking for a few years ago. This will allow you to kill the process on throws.

Will you be able to hook in and trap an accidental exception (not an explicit throw) in the promise constructor? If not, that remains a use-case I haven't seen addressed in the post-summit discussions.

@benjamingr
Copy link
Member Author

Will you be able to hook in and trap an accidental exception (not an explicit throw) in the promise constructor? If not, that remains a use-case I haven't seen addressed in the post-summit discussions.

Do you mean something like https://github.com/nodejs/promise-use-cases/blob/master/use-cases/warnings/3/3.md ? I believe we've discussed it and agreed it would be interesting to make this detectible via tooling.

I'll edit it in, thanks for noticing

@getify
Copy link
Contributor

getify commented Jun 6, 2018

Do you mean something like ...

No, I mean this: https://github.com/nodejs/promise-use-cases/blob/master/use-cases/user-expectations/6/expectations-6.md

throw or reject(..) in a constructor, after resolution, is one use-case; in other words, intentional expected rejection-after-resolution. But the separate use-case I'm concerned about is trapping and dealing with unintentional, unexpected exceptions after resolution.

These two may currently have the same outcome, but my argument in that linked document is that the user actually has different expectations between them. We should ideally provide ways to not only trap both for both cases, but also identify which case has happened, and maybe even deal with them differently.

new Promise(function c(resolve){
   resolve(42);
   reject();
   throw "uncaught";
});
new Promise(function c(resolve){
   resolve(42);
   misspelled();
});

What I'm saying is, the unexpected/accidental JS exception in the latter snippet, thrown when I try to call a misspelled function, is something I'd like at a minimum to be able to trap for with a hook instead of it being swallowed... but ideally (as argued in that linked document), I'd liked to configure Promises via a flag so that this latter promise actually ended up rejected with the JS exception.

@yunong
Copy link
Member

yunong commented Jun 6, 2018

@benjamingr Let's set up a call to discuss further -- what times work for you?

@benjamingr
Copy link
Member Author

@yunong can you make a separate issue for postmortem? I think we should make a doodle to give other interested parties a chance to participate :)

@getify
Copy link
Contributor

getify commented Jun 6, 2018

I think it could furthermore be argued that both of these:

new Promise(function c(resolve){
   resolve(42);
   throw "oops";
});
new Promise(function c(resolve){
   resolve(42);
   oops();
});

Should result (perhaps through opt-in via a flag) in an "uncaughtException" event (not "unhandledRejection") being fired in Node.

By contrast, this:

new Promise(function c(resolve,reject){
   resolve(42);
   reject("oops");
})
.catch(e => e);

...could be argued should (via opt-in) still report an "unhandledRejection".

@benjamingr
Copy link
Member Author

@getify from what I understood neither would report an unhandledRejection or uncaughtException. Rather we'd get an async_hook that users could use to receive the event and either log it to an APM or to emit their choice or unhandledRejection uncaughtException, a warning or even a process.exit

@getify
Copy link
Contributor

getify commented Jun 6, 2018

@benjamingr OK, as long as the (second) snippet accidentally calling oops() invokes this hook in the same way that the other two snippets (one and three) do.

And ideally, that hook would receive some sort of signal or information that told you how it was raised -- via which of those 3 approaches? -- since you might want to do different things depending on how it was raised.

@benjamingr
Copy link
Member Author

benjamingr commented Jun 6, 2018

Got it, let's discuss this again once V8 work on their hooks reimplementation and support for this to discuss specifics.

(I am assuming oops is not defined right?)

@getify
Copy link
Contributor

getify commented Jun 6, 2018

(I am assuming oops is not defined right?)

It's a placeholder illustration of any sort of valid JS code that for whatever reason causes an unexpected runtime JS exception, such as a TypeError or ReferenceError.

@targos
Copy link
Member

targos commented Jan 2, 2019

Closing this issue because the repository is being archived: nodejs/admin#283

@targos targos closed this as completed Jan 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants