{"action":"created","issue":{"url":"https://api.github.com/repos/nodejs/post-mortem/issues/16","repository_url":"https://api.github.com/repos/nodejs/post-mortem","labels_url":"https://api.github.com/repos/nodejs/post-mortem/issues/16/labels{/name}","comments_url":"https://api.github.com/repos/nodejs/post-mortem/issues/16/comments","events_url":"https://api.github.com/repos/nodejs/post-mortem/issues/16/events","html_url":"https://github.com/nodejs/post-mortem/issues/16","id":131795848,"number":16,"title":"promises and post-mortem debugging","user":{"login":"misterdjules","id":811047,"avatar_url":"https://avatars.githubusercontent.com/u/811047?v=3","gravatar_id":"","url":"https://api.github.com/users/misterdjules","html_url":"https://github.com/misterdjules","followers_url":"https://api.github.com/users/misterdjules/followers","following_url":"https://api.github.com/users/misterdjules/following{/other_user}","gists_url":"https://api.github.com/users/misterdjules/gists{/gist_id}","starred_url":"https://api.github.com/users/misterdjules/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/misterdjules/subscriptions","organizations_url":"https://api.github.com/users/misterdjules/orgs","repos_url":"https://api.github.com/users/misterdjules/repos","events_url":"https://api.github.com/users/misterdjules/events{/privacy}","received_events_url":"https://api.github.com/users/misterdjules/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":80,"created_at":"2016-02-06T00:19:54Z","updated_at":"2016-02-12T01:36:44Z","closed_at":null,"body":"A [PR](https://github.com/nodejs/node/pull/5020) that aims at adding a promises-based API to most node core APIs was recently submitted to nodejs/node.\r\n\r\nI have [expressed my concerns](https://github.com/nodejs/node/pull/5020#issuecomment-180022665) regarding the impact of such a change on post-mortem debugging, and [a separate issue](https://github.com/nodejs/node/issues/5084) was created by @Raynos to further discuss these concerns.\r\n\r\nMy concerns are more questions that I asked because I felt that the way promises handle errors is incompatible, at least in some ways, with post-mortem debugging. So far my investigations only confirmed these concerns, but I'm not done with them. One part of these investigations is also to find a solution to the problems that I found so far.\r\n\r\nThe discussion in nodejs/node has been productive so far, but I represent only one post-mortem debugging user/maintainer. I feel it would help if some people from this working group would share their opinion on these concerns with the rest of the group here in this issue, if only to confirm or infirm that they have the same questions/reserves as I do about the impact of https://github.com/nodejs/node/pull/5020 on post-mortem debugging. For instance, if some of you already worked on that problem and did a similar investigation, I would be interested if you could share your results.\r\n\r\nIn any case, I will share my findings with the group as soon as I have some more time to continue my investigations.\r\n\r\nThank you!"},"comment":{"url":"https://api.github.com/repos/nodejs/post-mortem/issues/comments/183144104","html_url":"https://github.com/nodejs/post-mortem/issues/16#issuecomment-183144104","issue_url":"https://api.github.com/repos/nodejs/post-mortem/issues/16","id":183144104,"user":{"login":"chrisdickinson","id":37303,"avatar_url":"https://avatars.githubusercontent.com/u/37303?v=3","gravatar_id":"","url":"https://api.github.com/users/chrisdickinson","html_url":"https://github.com/chrisdickinson","followers_url":"https://api.github.com/users/chrisdickinson/followers","following_url":"https://api.github.com/users/chrisdickinson/following{/other_user}","gists_url":"https://api.github.com/users/chrisdickinson/gists{/gist_id}","starred_url":"https://api.github.com/users/chrisdickinson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/chrisdickinson/subscriptions","organizations_url":"https://api.github.com/users/chrisdickinson/orgs","repos_url":"https://api.github.com/users/chrisdickinson/repos","events_url":"https://api.github.com/users/chrisdickinson/events{/privacy}","received_events_url":"https://api.github.com/users/chrisdickinson/received_events","type":"User","site_admin":false},"created_at":"2016-02-12T01:36:44Z","updated_at":"2016-02-12T01:36:44Z","body":"Cross-linking [my latest comment from the symposium repo](https://github.com/groundwater/nodejs-symposiums/pull/5#issuecomment-183081930). I'm going to \r\n\r\nTo frame things a bit: we are going to have to make modifications to V8 in any case, for exactly the reasons that @bmeck touches on — namely, we don't have enough control over the `MicrotaskQueue` for native Promises at present, and that breaks `AsyncWrap`, which we absolutely want to ship. I am currently working on this, and it's a blocker for both unflagging the promise API as well as exposing the AsyncWrap API.\r\n\r\nIn my linked comment, I explain that subclassing+patching `Promise` is an option of last resort for core — I tried to go down that road to fix asyncwrap and promises, and it's prohibitively difficult to do it without the leaking out details of the original native `Promise`. Absent patching `Promise` entirely, returning a specialized subclass from a Core Promise API is a no-go — this precludes use cases that we want to support, like swapping out the promise implementation at app level.\r\n\r\nI believe I have come around to the point of view that Node should crash on `unhandledRejection` by default if no handler is installed, since existing package authors can't rely on their consumers _not_ to crash the program on `unhandledRejection` anyway. This aligns `unhandledRejection` with `uncaughtException`, also.\r\n\r\nI see two potential paths forward to implementing a \"crash by default on unhandled rejection\".\r\n\r\n#### Approach 1\r\n\r\nThis approach is close to the status quo. It leaves users the ability to reject synchronously, at the expense of making the program behave differently under the `--abort-on-unhandled-rejection` flag — notably, with the flag enabled the program crashes earlier than it otherwise would.\r\n\r\n1. A promise rejects and is unhandled.\r\n2. If `--abort-on-unhandled-rejection` is **not** set,\r\n  1. on `nextTick`, check to see if the promise is still rejected. If it is,\r\n    1. Fire the `unhandledRejection` event. If a listener fires,\r\n      1. **continue normally.**\r\n    2. Otherwise,\r\n      1. **crash the program.**\r\n  2. If it isn't,\r\n    1. **continue normally.**\r\n3. Otherwise, if `--abort-on-unhandled-rejection` **is** set,\r\n  1. **immediately `abort()`**\r\n    * Assuming the `PromiseHasUserDefinedRejectHandler` changes from @misterdjules land in V8, the crash will happen at top of stack.\r\n\r\n#### Approach 2\r\n\r\nThis approach aligns the behavior of Node running with and without the `--abort-on-unhandled-rejection` flag, at the expense of making synchronous rejection immediately crash the program. This could be mitigated somewhat by patching the `Promise.reject` helper to reject on next tick, but it still has some interesting repercussions for, e.g., programmer errors due to bad arguments passed to the Node API.\r\n\r\n1. A promise rejects and is unhandled.\r\n2. If `--abort-on-unhandled-rejection` is **not** set,\r\n  1. Fire the `unhandledRejection` event. If a listener fires,\r\n    1. **continue normally.**\r\n  2. Otherwise,\r\n    1. **crash the program.**\r\n3. Otherwise, if `--abort-on-unhandled-rejection` **is** set,\r\n  1. **immediately `abort()`**\r\n    * Assuming the `PromiseHasUserDefinedRejectHandler` changes from @misterdjules land in V8, the crash will happen at top of stack.\r\n\r\nThe question is: is it acceptable to preclude catching of synchronous rejections? For example, the Node API will synchronously reject if called with bad parameters. Under this scheme, I believe there would be no way to catch that error in-situ — it would immediately crash the program unless a user installed a `unhandledRejection` handler. For example in:\r\n\r\n```js\r\ntry {\r\n  const data = await fs.readFile(null)\r\n} catch (err) {\r\n  console.error('never reached')\r\n}\r\n```\r\n\r\nWe would never reach `'never reached'` unless a `unhandledRejection` handler was installed. _Is this acceptable?_ It seems to have some desirable elements for post mortem & error symposium users. My gut feeling is that synchronous rejection is enough of a corner case that it might be valuable to repurpose it in this fashion. On the other hand, a user running with `--abort-on-uncaught-exception` is likely to _always_ run their application with the flag, so perhaps the slightly different behavior is worth the price of not having uncatchable, fatal exceptions by default?\r\n\r\n/cc'ing @fishrock123 since IIRC he's looking into making unhandled rejections crash when the promise is GC'd. If we go with one of the approaches above we won't need to do that; however it would be good to get his input!"}}