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

Discuss blocking on Zones proposal #340

Closed
bmeck opened this issue Sep 8, 2017 · 29 comments
Closed

Discuss blocking on Zones proposal #340

bmeck opened this issue Sep 8, 2017 · 29 comments

Comments

@bmeck
Copy link
Member

bmeck commented Sep 8, 2017

Moving here, as per: nodejs/CTC#166 (comment)

https://github.com/domenic/zones is a proposal that is currently being blocked by Node.

The proposal is something similar to domains and async_hooks in creating a way to pass data through asynchronous tasks.

The blocker has been the handling of errors with the proposal.

I would like to setup a meeting to discuss the blocking behavior now that async_hooks is landed.

@Trott
Copy link
Member

Trott commented Sep 8, 2017

@nodejs/async_hooks I guess?

@AndreasMadsen
Copy link
Member

We can setup a meeting in the diagnostics WG.

Does this have any relation to angular/zone.js#795? /cc @JiaLiPassion

@JiaLiPassion
Copy link

@AndreasMadsen , thanks for cc me, the nodejs/CTC#166 (comment) seems about error handling of zone proposal, it is not directly related to angular/zone.js#795, but I would like to provide any help I can.

@bmeck
Copy link
Member Author

bmeck commented Sep 9, 2017

@JiaLiPassion Is the following no longer a goal?

  1. Error
    use process.unCaughtException to redirect to ZoneDelegate.handleError.

@JiaLiPassion
Copy link

@bmeck ,

  1. use process.unCaughtException to redirect to ZoneDelegate.handleError is still a goal, to use async_hooks to implements zone, this is the only way I found to handle error.

  2. currently the [WIP] feat(async): fix #740, use async hooks to wrap nodejs async api angular/zone.js#795 is stopped because I can't find a way to implement zone aware promise with current async_hooks API.

@mcollina
Copy link
Member

I am 👎 on changing process.on('uncaughtException') default behavior: crashing the process. It should be possible to enable zones if someone wants to, but a lot of modules and applications out there are developed thinking that an error will crash the process.

Generically, I would prefer that zones would create a new context that does not influences anything that happens outside of it.

@JiaLiPassion
Copy link

@mcollina , thank you for your advice, currently, my thought is just use async_hooks to simulate original zone.js monkey-patch behavior.

  • with zone.js monkey-patch version. zone.js intercept all async callback, so it will be able to catch all errors.

  • with async_hooks which is just a notification not an interception, so it is not possible to catch any errors.

So currently I just thought maybe with process.on('unaughtException'), it is a way to do what currently zone.js does.

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Sep 12, 2017

Generically, I would prefer that zones would create a new context that does not influences anything that happens outside of it.

Unfortunately, we don't have a mechanism for that. Domains works by catching the error beforeunaughtException, likely zones will work that way too.

@JiaLiPassion if you want to experiment with it, you can overwrite process._fatalException or process.domain._errorHandler. it is not a public API so there are never any guarantees but process._fatalException especially is not something that is likely to change.

@JiaLiPassion
Copy link

@AndreasMadsen , thank you ! I will try it.

@MylesBorins
Copy link
Contributor

Hey all, is there an update on this? should we consider a strategic initiative around it?

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Dec 11, 2017

@JiaLiPassion We now have a public API for catching error the way domain does it. And we have migrated the domain module to use the API.

Additionally, there is a general tracking issue for implementing domain using only public APIs nodejs/node#17143

@JiaLiPassion
Copy link

@AndreasMadsen , Thank you for the update, I will look into it. And I will also check the never resolved promise issue.

@AndreasMadsen
Copy link
Member

@JiaLiPassion we now have the promiseResolve event. so that issue should be possible to solve now too :)

@JiaLiPassion
Copy link

@AndreasMadsen , got it, I will check it, thank you!!!

is the new promiseResolve can handle this case?

 const p1 = new Promise(() => {}); // This will never be resolved
 const p2 = new Promise((resolve) => setTimeout(resolve, 1000, p1)); // resolve p2 with p1 after 1s

I will test it.

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Dec 14, 2017

@JiaLiPassion I'm not sure it can. I think it just emits the first resolve value. It would be smart if we could know the "resolved" value.

@JiaLiPassion
Copy link

@AndreasMadsen , got it, I will test it.

@jasnell
Copy link
Member

jasnell commented Feb 17, 2018

Discussion here appears to have stalled. Is there a next step or can we close this issue?

@bmeck
Copy link
Member Author

bmeck commented Feb 17, 2018

@jasnell I've never gotten a clear statement on if we want to continue to block the language level feature still. It seems ok to close, but I suspect this issue will continue to be raised at a language level.

@jasnell
Copy link
Member

jasnell commented Feb 17, 2018

yeah, I doubt this issue is going to be enough to get the existing TSC membership organized enough to form a coherent opinion. If there is forward movement on the zones proposal, we can review at that time, I suppose.

@bmeck
Copy link
Member Author

bmeck commented Feb 17, 2018

@jasnell there is no momentum / Zones is stalled due to block from Node on Error handling ( domenic/zones#9 ), the champion sees Error handling as mandatory to move forward.

@jasnell jasnell removed the stalled label Feb 17, 2018
@jasnell
Copy link
Member

jasnell commented Feb 17, 2018

Ok, how about sometime in the next few months we queue this back up for a more active TSC discussion?

@jasnell
Copy link
Member

jasnell commented Apr 6, 2018

@nodejs/tsc ... @mcollina and I have been discussing the issues around Zones and some of the requirements around continuation local storage in general. We've been kicking around a few ideas that I've captured here: https://docs.google.com/presentation/d/1N8w-Ees6sy5qcqh8y5Fc271pcicbI4AB1D8eCIl_1aQ/edit?usp=sharing

I'd like to get some discussion going among ourselves to see if this is an approach we could rally behind before taking it to TC-39.

@fhinkel
Copy link
Member

fhinkel commented Apr 23, 2018

@jasnell was this discussed at the last TC39 meeting?

@ljharb
Copy link
Member

ljharb commented Apr 24, 2018

@fhinkel it was not discussed in March; the next TC39 meeting after that comment was posted will be at the end of May.

@fhinkel
Copy link
Member

fhinkel commented Apr 24, 2018

@ljharb Thanks!

@jasnell, does this need to be discussed by the TSC or would @nodejs/diagnostics be a better place to handle this?

@jasnell
Copy link
Member

jasnell commented Apr 24, 2018

Yes. Both :)

@mike-kaufman
Copy link

If someone could tag this with diag-agenda, we'll bring it up at next diag WG meeting. Here's my .02, plz let me know if you disagree 😄 :

There are two fundamental problems w/ "async" in javascript:

  • Problem 1: Shared semantics & terminology. My read on why the Zones proposal bogged down, is there's no way to talk about & reason about asynchronous execution in JavaScript. There's lots of different strategies that can be applied to how an error propagates across an async call graph, and different approaches have different tradeoffs, but being able to discuss these at the right level of abstraction is critical. We've updated some of the terminology we've been working on here, and would appreciate any feedback.

  • Problem 2: Understanding async boundaries in user code. For the javascript programmer, understanding async boundaries is difficult, primarily because there's no syntactic construct in JS code to understand where these occur.

It is our belief (hope?) that if we get the concepts & terminology in 1) correct, then language constructs and VM support should follow easily.

RE Zones, it provides some capabilities over the async call graph, namely continuation-local-storage and error propagation. However, there are other desired capabilities as well, e.g., post-mortem analysis, or a debugger stepping across async boundaries "in context".

Given all of this, I think Zones is a bit premature. If something goes to TC39, I would prefer it focus on first-principles and address those two fundamental problems above. If we can get those things nailed, then we can talk about user APIs to support CLS or error async error propagation or anything else. Moreover, with the right model in place, if someone doesn't like Zones behavior, they're free to implement their own over the primitives.

/cc @bterlson, @mrkmarron

@mhdawson
Copy link
Member

This was discussed in the latest Diagnostic WG meeting. At this point we believe we need to close on the work on async formalization before we might look at something like this. Given that and that we have nobody stepping up as a champion going to remove from diagnostic agenda for now.

@Trott
Copy link
Member

Trott commented Oct 2, 2018

Closing this due to inactivity and it likely being out-of-date, but feel absolutely free to hit the re-open button if that's not the right thing to do.

@Trott Trott closed this as completed Oct 2, 2018
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