{"action":"created","issue":{"url":"https://api.github.com/repos/domenic/zones/issues/9","repository_url":"https://api.github.com/repos/domenic/zones","labels_url":"https://api.github.com/repos/domenic/zones/issues/9/labels{/name}","comments_url":"https://api.github.com/repos/domenic/zones/issues/9/comments","events_url":"https://api.github.com/repos/domenic/zones/issues/9/events","html_url":"https://github.com/domenic/zones/issues/9","id":148468917,"number":9,"title":"RFC: adding built-in error handling support to zones","user":{"login":"domenic","id":617481,"avatar_url":"https://avatars.githubusercontent.com/u/617481?v=3","gravatar_id":"","url":"https://api.github.com/users/domenic","html_url":"https://github.com/domenic","followers_url":"https://api.github.com/users/domenic/followers","following_url":"https://api.github.com/users/domenic/following{/other_user}","gists_url":"https://api.github.com/users/domenic/gists{/gist_id}","starred_url":"https://api.github.com/users/domenic/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/domenic/subscriptions","organizations_url":"https://api.github.com/users/domenic/orgs","repos_url":"https://api.github.com/users/domenic/repos","events_url":"https://api.github.com/users/domenic/events{/privacy}","received_events_url":"https://api.github.com/users/domenic/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":46,"created_at":"2016-04-14T20:09:32Z","updated_at":"2016-05-17T23:33:25Z","closed_at":null,"body":"This is a request for comments, especially from the Node community (/cc @bmeck @trevnorris; please bring in others). Our original plan of leaving error handling out of the base zone primitive, and letting it be handled entirely by host environments or future spec extensions, is starting to show some cracks. This is mainly because it impacts the recommendations around how to \"wrap\" functions for zones. We would like the base zones proposal to have a strong recommendation: \"to properly propagate zones while doing async work, do X.\"\r\n\r\n#### The problem\r\n\r\nCurrently our recommendation for how to propagate zones is \"use `Zone.current.wrap`\", but @mhevery has shown me how that doesn't quite work in certain scenarios once error handling is introduced. In *most* cases of user-mode queuing, you want wrapped functions to send any errors to the relevant zone. However, in *some* cases, mainly when building control flow abstractions like promise libraries, you want to handle errors yourself.\r\n\r\nSo, if we later introduced error handling, some fraction of the uses of `Zone.current.wrap` would be wrong.\r\n\r\nSimilar problems apply to `zone.run`, since one of the important uses of `run` for library authors is to manually do wrapping (i.e., store the callback and current zone, then do `storedZone.run(storedCallback)`, instead of storing the wrapped callback).\r\n\r\n#### The solution\r\n\r\nThe way to fix this is to make the two use cases explicit. Instead of `wrap` and `run`, we have `wrap`, `wrapGuarded`, `run`, and `runGuarded`, where the non-guarded variants are used when the library explicitly wants to handle thrown exceptions: like for implementing promises or observables or similar, where you transform thrown exceptions into a different form.\r\n\r\nHowever, it's pretty pointless to introduce these two functions if the zones proposal doesn't actually have error handling built-in. So this takes us down the path of introducing error handling into the base zone primitive, instead of saving it for a future extension.\r\n\r\nIn other words, keeping things in a future extension is fine, unless that requires you to change your code now. If you require people to change their code now, you might as well give them the benefits (zone error handling) that you're asking them to pay for.\r\n\r\n#### Details of error handling proposal\r\n\r\nThe zone fork options get a `handleError` option, which takes a thrown error object:\r\n\r\n```js\r\nconst z = Zone.current.fork({\r\n  name: 'http request zone',\r\n  handleError(e) {\r\n    sendAnalytics(e);\r\n    return true; // error handled. Or, should we just make you rethrow to indicate not-handled?\r\n  }\r\n});\r\n```\r\n\r\nThis is pretty simple. The trick is then figuring out when and how we should route errors to the error handler. To discuss that, we need to talk about the “guarded” functions introduced above.\r\n\r\n#### Details of {wrap,run}{Guarded}\r\n\r\nCurrently, we have (essentially)\r\n\r\n````js\r\nZone.prototype.run = function (f) { // current\r\n  const oldZone = Zone.current;\r\n  setCurrentZone(this); // privileged API\r\n  try {\r\n    return f();\r\n  } finally {\r\n    setCurrentZone(oldZone);\r\n  }\r\n};\r\n\r\nZone.prototype.wrap = function (f) {\r\n  const thisZone = this;\r\n  return function () {\r\n    return thisZone.run(() => f.apply(this, arguments));\r\n  };\r\n};\r\n```\r\n\r\nWe would then introduce:\r\n\r\n```js\r\nZone.prototype.runGuarded = function (f) { // new\r\n  const oldZone = Zone.current;\r\n  setCurrentZone(this); // privileged API\r\n  try {\r\n    return f();\r\n  } catch (e) {\r\n    // actually stored in an internal slot, not a _-prefixed property\r\n    if (!this._handleError || !this._handleError(e)) {\r\n      throw e;\r\n    }\r\n  } finally {\r\n    setCurrentZone(oldZone);\r\n  }\r\n};\r\n\r\nZone.prototype.wrapGuarded = function (f) {\r\n  const thisZone = this;\r\n  return function () {\r\n    return thisZone.runGuarded(() => f.apply(this, arguments));\r\n  };\r\n};\r\n```\r\n\r\n#### How to think about these\r\n\r\nThe TL;DR is: most libraries use wrapGuarded. Most apps use run.\r\n\r\nIn a bit more detail: the user code and the library collaborate to figure out how errors are handled, with the following inputs:\r\n\r\n- Does the library know how to handle errors? If so, it will use wrap (or run), in combination with a try/catch at the call site. Otherwise (most cases, like setTimeout or event listeners), it will use wrapGuarded (or runGuarded), to say: “I don’t know how to handle the error, and if I let it propagate up the call site it would just immediately reach top level, so instead let’s route errors to the right zone.”\r\n- Does the user code want the zone to handle errors? If so, it will use runGuarded to kick things off. Otherwise, it will use run, and let the error propagate up the call stack as usual. Using runGuarded in this way is fairly unusual; it’s a kind of “async try/catch” and programs are generally better served by letting the error bubble.\r\n\r\n#### Example in action\r\n\r\nThis example shows how, if you follow the TL;DR above, everything “just works”:\r\n\r\n```js\r\n/////// LIBRARY CODE\r\n\r\nsql.addListener = function (f) {\r\n  this.storedFunction = Zone.current.wrapGuarded(f);\r\n};\r\n\r\nsql.doStuff = function () {\r\n  this.storedFunction();\r\n};\r\n\r\n/////// APP CODE\r\n\r\nconst rootZone = Zone.current;\r\n\r\nconst zone1 = rootZone.fork({\r\n  handleError: handleError1\r\n});\r\nconst zone2 = rootZone.fork({\r\n  handleError: handleError2\r\n});\r\n\r\nzone1.run(function a() {\r\n  sql.addListener(function b() {\r\n    rootZone.run(function c() {\r\n      throw new Error(\"boo\");\r\n    });\r\n  });\r\n});\r\n\r\nzone2.run(function d() {\r\n  setTimeout(function e() {\r\n    sql.doStuff();\r\n  }, 0);\r\n});\r\n```\r\n\r\nAt the time the error is thrown, the call stack is:\r\n\r\n - `c` (top)\r\n - `rootZone.run`\r\n - `b`\r\n - `this.storedFunction` (wrapper around `b` to run it in zone1)\r\n - `sql.doStuff`\r\n - `e\r\n - wrapper around `e` to run it in `zone2` (generated by setTimeout)\r\n - setTimeout task (bottom)\r\n\r\nError propagation and handling then occurs like so:\r\n\r\n- The error is not caught by `rootZone.run` (`run` does not handle errors at all).\r\n- The error is next caught by `this.storedFunction`, which is a wrapper around `b` to run it guarded in zone1. That sends it to `errorHandler1`.\r\n- If `errorHandler1` doesn't return true, the error is next caught by the wrapper around `e` to run it in `zone2`. So it's sent to `errorHandler2`.\r\n- If even `errorHandler2` doesn't try it, we call the error unhandled, and it goes to `window.onerror` or `\"uncaughtException\"` as usual.\r\n\r\n#### Issues to discuss\r\n\r\nMost importantly: does this sound like something that is acceptable to potential zone-using communities? We’d like to have everyone on board, and we spent a lot of time trying to get the details right here (drawing on things like the [domain module postmortem](https://gist.github.com/trevnorris/d19addf4aa09a0a31a4a14e90cb5c302) from Node.js), so hopefully it’s not that bad.\r\n\r\nLess important issues:\r\n\r\n- How should unhandled/handled be done? I did returning true for handled, false for unhandled, but I think another plausible design is that it’s handled by default, and you rethrow the error if you want it to be unhandled. (Assume for the purposes of discussion that we properly specify Error.prototype.stack as being captured at error creation time, not at error throwing time, so rethrowing does not hurt stack information.)\r\n\r\n- Should we consider only adding run + wrapGuarded, since those are the “happy path”? I think it’s weird to have an asymmetric API like that, and there are definitely use cases for the other two. But on the other hand, you can derive runGuarded from wrapGuarded, and derive wrap from run, so it’s not *necessary*.\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/domenic/zones/issues/comments/219884945","html_url":"https://github.com/domenic/zones/issues/9#issuecomment-219884945","issue_url":"https://api.github.com/repos/domenic/zones/issues/9","id":219884945,"user":{"login":"mhevery","id":111951,"avatar_url":"https://avatars.githubusercontent.com/u/111951?v=3","gravatar_id":"","url":"https://api.github.com/users/mhevery","html_url":"https://github.com/mhevery","followers_url":"https://api.github.com/users/mhevery/followers","following_url":"https://api.github.com/users/mhevery/following{/other_user}","gists_url":"https://api.github.com/users/mhevery/gists{/gist_id}","starred_url":"https://api.github.com/users/mhevery/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mhevery/subscriptions","organizations_url":"https://api.github.com/users/mhevery/orgs","repos_url":"https://api.github.com/users/mhevery/repos","events_url":"https://api.github.com/users/mhevery/events{/privacy}","received_events_url":"https://api.github.com/users/mhevery/received_events","type":"User","site_admin":false},"created_at":"2016-05-17T23:33:25Z","updated_at":"2016-05-17T23:33:25Z","body":"> Cool. So take const global_zone = Zone.current; at bootstrap then do global_zone.run() in require()?\r\n\r\n`Zone.current.root` gives you root.\r\n\r\n\r\n\r\n> I'm not sure how to determine if a Zone is the root other than by name. Does this mean creating a root with the name '(root zone)' would throw, or there some other mechanism to determine this?\r\n\r\nEach Zone has a parent. The root zone is the one which has no parent, and behaves indistinguishable from the current platform behavior. Only the platform is allowed to create a root zone, everyone else must fork.\r\n\r\n=======\r\n\r\nI am a bit confused about your first example. You set up `z1` which swallows exceptions, and then you are concerned that exceptions get swallowed? How is this different from setting `uncaughtException` to ignore errors?\r\n\r\nWhy do you want `mlib.js` to create zones in first place? Is your goal to do error logging? (I guess my assumption is that forking zones should be pretty rare occurrence, which I don't think you share)\r\n\r\nIn my mind the only time a library should fork or capture a zone is if it does user queue operations. (Such as implementing work queue)\r\n\r\nCould we move this discussion from hypothetical here-is-how-i-can-break-zones to concrete use cases where zones fails to perform as expected. Sorry, it is hard for me to follow, and I think the discussion would be more productive."}}