{"action":"created","issue":{"url":"https://api.github.com/repos/nodejs/node/issues/22822","repository_url":"https://api.github.com/repos/nodejs/node","labels_url":"https://api.github.com/repos/nodejs/node/issues/22822/labels{/name}","comments_url":"https://api.github.com/repos/nodejs/node/issues/22822/comments","events_url":"https://api.github.com/repos/nodejs/node/issues/22822/events","html_url":"https://github.com/nodejs/node/issues/22822","id":359534426,"node_id":"MDU6SXNzdWUzNTk1MzQ0MjY=","number":22822,"title":"[Discuss] Resolving the unhandled rejections issue","user":{"login":"jasnell","id":439929,"node_id":"MDQ6VXNlcjQzOTkyOQ==","avatar_url":"https://avatars2.githubusercontent.com/u/439929?v=4","gravatar_id":"","url":"https://api.github.com/users/jasnell","html_url":"https://github.com/jasnell","followers_url":"https://api.github.com/users/jasnell/followers","following_url":"https://api.github.com/users/jasnell/following{/other_user}","gists_url":"https://api.github.com/users/jasnell/gists{/gist_id}","starred_url":"https://api.github.com/users/jasnell/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jasnell/subscriptions","organizations_url":"https://api.github.com/users/jasnell/orgs","repos_url":"https://api.github.com/users/jasnell/repos","events_url":"https://api.github.com/users/jasnell/events{/privacy}","received_events_url":"https://api.github.com/users/jasnell/received_events","type":"User","site_admin":false},"labels":[{"id":171787081,"node_id":"MDU6TGFiZWwxNzE3ODcwODE=","url":"https://api.github.com/repos/nodejs/node/labels/discuss","name":"discuss","color":"02d7e1","default":false},{"id":340436277,"node_id":"MDU6TGFiZWwzNDA0MzYyNzc=","url":"https://api.github.com/repos/nodejs/node/labels/promises","name":"promises","color":"f7c6c7","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":11,"created_at":"2018-09-12T15:26:24Z","updated_at":"2018-09-13T07:00:18Z","closed_at":null,"author_association":"MEMBER","body":"@nodejs/tsc @nodejs/collaborators (yes, pinging @nodejs/collaborators is intentional and yes, I know it causes lots of people to get notifications... that's what I want because I want broad input into this conversation)\r\n\r\nWe currently have two PRs open that deal with unhandled promise rejections.\r\n\r\n* https://github.com/nodejs/node/pull/22218\r\n* https://github.com/nodejs/node/pull/21857\r\n\r\nBoth do or assume several things:\r\n\r\n1. Both introduce APIs that users may use to observe `Promise` state.\r\n2. Both make *assumptions* (albeit very different assumptions) about what the proper handling of unhandled rejections by Node.js should be.\r\n\r\nThese PRs have been generally treated as mutually exclusive by the authors and the discussion about the introduced APIs has been generally held up by disagreements in (a) the fundamental handling of unhandled rejections and (b) whether or not Node.js should warn users about potentially [problematic coding patterns](https://github.com/nodejs/node/issues/22587). Unfortunately, what I see happening now is that issues that *should* be separate threads of discussion are being lumped all in together. So I'd like to try to separate these things out.\r\n\r\n## Warn or Not Warn\r\n\r\nLet's take the whole discussion about `Promises` out of the picture for now. There is a general philosophical/values discussion to be had about whether or not it is Node.js' responsibility to warn users about potentially problematic coding patterns that may exist. It has been argued by a couple of folks that it is not Node.js' place to \"judge\" someones code. This has been expanded to mean that when the user runs some code that is not ideal but otherwise runs, Node.js should just stay silent on the matter.\r\n\r\nAn example of this is resolving a Promise twice... e.g.\r\n\r\n```js\r\nnew Promise((resolve, reject) => {\r\n  resolve('A')\r\n  resolve('B')\r\n});\r\n\r\nor \r\n\r\nnew Promise((resolve, reject) => {\r\n  reject(new Error('A'));\r\n  resolve('B');\r\n});\r\n``` \r\n\r\nIn both cases, the `resolve('B')` is extraneous and will be swallowed by default by V8. The code will run but the second resolve is effectively a non-op. In a handful of cases, this situation *could* be intentional or *could* be a legitimate coding bug. Take the following, for instance... a forgotten `break` statement leads to the swallowed resolve:\r\n\r\n```js\r\nlet foo = getFoo()\r\nnew Promise((resolve, reject) => {\r\n  switch (foo) {\r\n    case 'A': resolve('A')\r\n    case 'B': resolve('B')\r\n  }\r\n});\r\n```\r\n\r\nWhen we look at this, we can likely reason that this is clearly a programmer error, but the code will run just fine.\r\n\r\nThanks to an internal callback API provided to us by V8, Node.js *can* detect when this secondary resolve or rejection occurs. The question is, should Node.js, by default:\r\n\r\n1. Emit a process warning letting the user know that this situation occurred, or\r\n2. Invoke an optional callback API that the user must explicitly opt in to using to be notified about this situation\r\n3. Both\r\n\r\nNote that when the `process.emitWarning()` API was added back in the Node.js 6.0, I took the approach of allowing *both*. By default, process warnings are emitted to stderr. Usercode can, however, listen to the `process.on('warning')` event and use the `--no-warnings` command-line flag to disable the default handling and provide their own handling of the process warning events. Because runtime deprecations now use the process warning mechanism, runtime deprecation warnings can be handled in the same way.\r\n\r\nFor warnings about synchronous file system I/O, however, we take a different approach. Synchronous I/O can be a significant performance bottleneck in Node.js and is generally considered to be an antipattern, but there are perfectly legitimate use cases for it. Therefore, we do nothing by default when sync I/O is used but we provide an opt in command line flag `--trace-sync-io` that users can switch on to help identify where sync io may be happening within their applications.\r\n\r\nGiven precedent, I don't think we can argue that there is any single rule of thumb to be applied here. Whether or not we warn by default can really only be determined on a case by case basis determined by how useful, or how problematic, a given coding pattern may be. So with that, we need to determine if swallowed resolves/rejections are potentially problematic enough to warrant a warning by default.\r\n\r\n## Crash or Not Crash\r\n\r\nWhen a `Promise` is rejected without a `catch` handler in place, what do we do? This has been a long raging discussion for several years now without any clear resolution. Some feel we should crash immediately, some feel we should ignore and just let it happen. Neither are wrong, neither are entirely correct. I would submit that this is something that Node.js should allow users to configure with a command line argument, but then the argument turns to what should a reasonable default among several options be. That's what we need to figure out here. For my money, crashing on garbage collection is not ideal but it's likely the best reasonable default option we have at this time so long as we also have the option to crash immediately or not at all available for users to select.\r\n\r\n## One API to Rule Them All\r\n\r\nThe two PRS currently open propose two different APIs users may use to get notified about Promise state.\r\n\r\nThe \"swallowed resolves\" PR (#22218) takes the approach of building on the existing `process.on('unhandledRejection')` event and adds a new `process.on('multipleResolves')` event. \r\n\r\nFor example:\r\n\r\n```js\r\nfunction handler(type, p, reason, message) {\r\n  // do something here\r\n}\r\n process.on('multipleResolves', handler);\r\n\r\n new Promise((resolve, reject) => {\r\n  resolve('A');\r\n  resolve('B');\r\n  reject(new Error('C'));\r\n});\r\n```\r\n\r\nThe \"promise events API\" PR (#21857) takes a more diagnostics API approach that adds a low level API for monitoring promise state. The basic idea would be to *replace* the existing `process.on('unhandledRejection')` event and to allow users to implement whatever handling they want:\r\n\r\n```js\r\nconst makeHook = (type) => (promise, value) => {\r\n  // do something here\r\n};\r\nutil.createPromiseHook({\r\n  resolve: makeHook('resolve'),\r\n  rejectWithNoHandler: makeHook('rejectWithNoHandler'),\r\n  handlerAddedAfterReject: makeHook('handlerAddedAfterReject'),\r\n  rejectAfterResolved: makeHook('rejectAfterResolved'),\r\n  resolveAfterResolved: makeHook('resolveAfterResolved'),\r\n}).enable();\r\n\r\nconst x = (async () => {\r\n  throw new Error('A');\r\n})();\r\n```\r\n\r\nEach of the APIs have their merits, and each have their warts. In the discussion threads for each of the PRs, the authors have argued their cases for why theirs in the right approach and we need to come to a conclusion on which API we want to move forward with. Given the disagreement thus far and the inability to come to resolution, the decision is likely to fall to the @nodejs/tsc unless the authors of the two PRs can come together and work through some compromise approach (which is what I would like to see).\r\n\r\nPersonally, I'm good with either of the two PRs. None of the arguments in favor of either has swayed me for or against either of them.\r\n\r\nWhen discussing this issue, please separate these three individual aspects so that any one of them does not get lost in the discussion."},"comment":{"url":"https://api.github.com/repos/nodejs/node/issues/comments/420904656","html_url":"https://github.com/nodejs/node/issues/22822#issuecomment-420904656","issue_url":"https://api.github.com/repos/nodejs/node/issues/22822","id":420904656,"node_id":"MDEyOklzc3VlQ29tbWVudDQyMDkwNDY1Ng==","user":{"login":"ronkorving","id":631240,"node_id":"MDQ6VXNlcjYzMTI0MA==","avatar_url":"https://avatars0.githubusercontent.com/u/631240?v=4","gravatar_id":"","url":"https://api.github.com/users/ronkorving","html_url":"https://github.com/ronkorving","followers_url":"https://api.github.com/users/ronkorving/followers","following_url":"https://api.github.com/users/ronkorving/following{/other_user}","gists_url":"https://api.github.com/users/ronkorving/gists{/gist_id}","starred_url":"https://api.github.com/users/ronkorving/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ronkorving/subscriptions","organizations_url":"https://api.github.com/users/ronkorving/orgs","repos_url":"https://api.github.com/users/ronkorving/repos","events_url":"https://api.github.com/users/ronkorving/events{/privacy}","received_events_url":"https://api.github.com/users/ronkorving/received_events","type":"User","site_admin":false},"created_at":"2018-09-13T07:00:18Z","updated_at":"2018-09-13T07:00:18Z","author_association":"CONTRIBUTOR","body":"@jasnell Excellent point. I guess then the question becomes: is the tooling *good enough*? IMHO, it's good enough to avoid having to make the case for a new dedicated event. I'll end my argument there :)"}}
{"actor":{"display_login":"ronkorving"}}