Differentiating Programmer Errors from Operational Errors

[chrisdickinson]

This primarily concerns the error symposium participants, but may also concern the post mortem WG.

The Problem

• Promise users expect all Promise-returning API usage to return a Promise object, success or fail.
• This means that invalid API use is returned as a rejection, alongside other errors.
• This necessitates the use of try / catch in async/await code.
• This also necessitates error type checking in promise-based code.
  • Were a "abort on unhandled rejection" flag to land, this would reduce such a flags efficacy: given that such a flag would operate by attempting to immediately abort the promise at top of stack assuming no user-installed handlers were present, and given that in type-checking users would usually add user-installed handlers.

    Proposed Solutions

    Recovery Object

Add an optional object parameter to all promise-returning APIs. This will be known as the recovery object. It does not change the behavior of Promises/A+; it is purely a Node API-level pattern. It allows users to intercept errors before settling the returned promise, and change the outcome of the lower-level API call:

// recovery object
await fs.readFilePromise('some/file', {
  ENOENT() {
    return null
  }
})

// normal use:
fs.readFilePromise('some/file')

Pros:

• Promise API users get the API they expect. Error symposium users get the API they expect.
• Simple to implement.

Cons:

• It's a new pattern.
• It doesn't solve problems in the ecosystem.

  --abort-on-sync-rejection

Add a flag to abort the process on synchronous new Promise rejection. This does not extend to synchronous throw within handlers. Potentially patch Promise.reject so that it returns a pending promise that rejects on next tick.

For example, the following usage would abort under this flag:

new Promise(() => { throw new Error() })
new Promise((_, reject) => { reject(new Error()) })

The following usage would not abort under this flag:

new Promise((_, reject) => { setTimeout(() => reject(new Error())) })
attainPromise().then(() => { throw new Error() })

The net effect being that the promise would immediately exit on invalid Node API use under this flag. Without the flag, the process keeps running "as expected" for the majority of promise users.

Pros:

• Solves problem in ecosystem for native promise users.
• Does not introduce a new pattern.

Cons:

• Unclear on how common synchronous rejection is in Promise constructors. Believed to be uncommon, could be proven wrong?
• Flagged and unflagged behavior diverge.

  This Discussion

Please pose problems with or benefits of the listed solutions, or propose alternate avenues of inquiry which will be added to the issue text. Clarifying comments, like "how are you using this?" are in-scope. Comments that suggest that the desire to separate operational errors from programmer errors is invalid will be moderated in the interest of keeping this discussion productive. Thank you for participating!

[phpnode]

--abort-on-sync-rejection sounds like it would be slightly more compatible with the ecosystem if it was only triggered on TypeError or ReferenceError.

I don't think introducing the new pattern from your first example is a good idea, and since it doesn't work with the ecosystem I don't think it's a valid option.

[chrisdickinson]

I don't think introducing the new pattern from your first example is a good idea, and since it doesn't work with the ecosystem I don't think it's a valid option.

To clarify, it doesn't solve the problem in-ecosystem for existing promise-based APIs. The abort-on-sync-rejection would only solve for native Promises. If possible, I'd like @donutespresso to weigh in on whether that's an acceptable trade off before we say the recovery object isn't a workable solution.

[benjamingr]

I'd appreciate it if we also consider the .recover extension I describe here:

let file = await fs.getFileAsync('dne').recover({
   EISDIR(err) {}
   ENOENT(err) { ... }
});

and copying your objections with replies here:

It puts core in the "Promise subclass" business, which I don't think we want to be in. Returning anything other than instances of the Promise class is liable to be confusing for users. Additionally, subclassing means that it becomes harder for application-level users to globally swap out Promise to their preferred implementation.

Why don't we want to be in it? Promises were built with subclassing ability exactly for us to be able to do it. It's a lot cleaner than patching promises, it allows reuse and so on. It lets recover chain cleanly and it allows using it in user code.

I'm not sure why it would be confusing to users, you can still globally swap out Promise and like I said if you support it in core we'll probably support it in bluebird too.

Since it uses .catch internally, it seems like it would be unsuitable for the purposes of post-mortem users (who look to be hitting on approach to abort on unhandled rejection while preserving stack when no user-installed catch handlers are installed.)

Why? It would use catch synchronously which is fine for post-mortem debugging unless I'm missing something. This is exactly the sort of context you'd want (handling the errors there). It would rethrow if no handler matched so it is still a synchronous throw in the asynchronous context. There is no magic or blessed core parameters - whatever solution we add to shim for no pattern-matching in catch I'd like to see it be built in a way that can be used in userland easily.

It moves the recovery object's handler method off the top of stack. This means that, should an author wish to abort in certain known operational error cases, the stack will be lost.

The stack would be exactly the same wouldn't it? Instead of a callback inside we take a callback outside.

For what it's worth, native promises are a little silly in that they always use the microtask queue. Bluebird for instance avoids it if it can prove the promise doesn't resolve synchronously anyway and avoids the extra queuing.

It looks like .recover() (vs. recovery-as-parameter) would get in the way of at least a few use cases, and put core in the business of augmenting the promise object, which I believe we do not wish to do.

Again, subclassing is not augmenting - it is extending. I'm not suggesting we alter the window.Promise object at all. I'm suggesting that instead of implementing this in about a hundred places for the entire core API we implement it once as a subclass.

---

Here is a fully concrete example you can run on your own computer, the actual example is at the bottom:

"use strict";
var fs = require("fs");

class CorePromise extends Promise {
    recover(o) {
        return this.catch(e => {
            if(o.hasOwnProperty(e.code) && typeof o[e.code] === "function") {
                return o[e.code].call(undefined, e);    
            } 
            return CorePromise.reject(e); // or `throw e`.
        });
    }
}

function readFilePromise(filename) { 
    // a terrible way to promisify, just for the example
    return new CorePromise((res, rej) => fs.readFile(filename, (err, data) => {
        if(err) return rej(err);
        else return res(data);
    }));
}

readFilePromise("nonExisting").recover({
    ENOENT(e) { console.error("Not Found :(", e.path) }
});
readFilePromise("actuallyADirectory").recover({
    // not caught, propagates to an unhandledRejection
    ENOENT(e) { console.error("Not Found :(", e.path) }
});
// this handler gets called for the directory since ENOENT does not match it.
process.on("unhandledRejection", (e) => console.error("Unhandled Rejection", e));

[petkaantonov]

Both (recover object and recover method) are non-standard promise API extensions that will be obsoleted when the standard catch statement is extended to support custom predicates

[chrisdickinson]

@benjamingr: To address subclassing:

1. One of the goals of adding a Promise API to core is to reduce friction by settling on a common-baseline Promise implementation. That is to say, instead of having to pick a shim and an implementation, a package author that desires to use promises need only pick an implementation if the baseline doesn't suit their needs. Returning a subclassed variant of Promise distances us from this goal by introducing a new type of promises alongside native promises — users have to determine whether or not they have a native promise or a subclassed CorePromise (this is what I mean by "augmenting" — not all promises have the same methods, and that can cause confusion.)
2. .recover changes the Promise API semantics, while a recovery object does not. There have been strong objections voiced about changing the semantics of promises in other threads.
3. .recover precludes aborting at top-of-stack for error symposium users.
4. Because CorePromise would extend global.Promise at startup time, and the promisify wrapper would hold a reference to CorePromise introduced by a variable binding, it would preclude a user from swapping out the promise implementation globally, because core APIs would continue to return CorePromise instead of their desired implementation. If we work to make it so that CorePromise can be swapped, then it limits valid implementations to those that include .recover.
5. As a platform, I would prefer to limit us to returning well-specified Promise objects by default — that is, Promise and CancellablePromise (in whatever form that lands in.) This means that creating our own CorePromise wouldn't be possible without a clear specification noting the behavior of CorePromise. This would also preclude us from returning ad-hoc .then-ables.

[spion]

@chrisdickinson here is, I think, where most promise users have the biggest disagreement.

Promise users do not find it necessary to distinguish between programmer vs operational errors. When using promises, there are only expected and unexpected errors and only the consumer should decide which errors are expected and which aren't.

I'm not going to discuss that in details here, the problem is that both promises and async await are designed with this assumption being fundamental.

As a result I think that to solve this issue we must discuss precisely why this distinction exists in node, what are the problems with try {} catch {} and whether promises perhaps solve those problems already, in a different way.

So far, the only real issue I've seen has been post-mortem debugging, but we may be able to solve that in a different way too (#8).

[chrisdickinson]

@spion I think @groundwater proves the exception to the assertion that promise users do not find differentiating these errors necessary. He is a user of promises via async/await, and finds it necessary to differentiate "I called the API incorrectly" from "this is a recoverable state of the program."

While promises (and async/await) are designed with the fundamental assumption that exceptional error flow is expected, I don't think that precludes us from accommodating the use case that @groundwater and the error symposium represents at the Node API (or platform) level.

[chrisdickinson]

@spion:

As a result I think that to solve this issue we must discuss precisely why this distinction exists in node, what are the problems with try {} catch {} and whether promises perhaps solve those problems already, in a different way.

I agree that we should have a discussion on this, but this thread is probably not the best place for it. As part of addressing #12 I will be PRing a document in summarizing their needs and desires. Would you be opposed to holding off on the discussion of the programmer/operational error distinction in this thread and deferring it to that PR? I will endeavor to have it up by end of day today.

[spion]

@chrisdickinson I think this is a good place, because if we determine that there are no reasons other than post-mortem debugging, and another solution for PMD is found, this issue would likely close.

Anyway, I wrote my argument here:

https://gist.github.com/spion/3a1cad2debc14c56f353

[phpnode]

@chrisdickinson the point is that we (promise users) do differentiate, with Bluebird we'd do something like this:

return request(url)
.then(storePageInADatabase)
.catch(Http400Exception, handleNotFound)
.catch(Http500Exception, handleServerError);

But of course this is not part of the promise spec. With async await,

try {
  const page = await request(url);
  const pageId = await storePageInADatabase(page);
  return pageId;
}
catch (e) {
  if (e instanceof Http400Exception)
    return handleNotFound(e);
  else if (e instanceof Http500Exception)
    return handleServerError(e);
  else 
    throw e; // Something we weren't expecting.
}

and in hypothetical pattern matching dream-land:

try {
  const page = await request(url);
  const pageId = await storePageInADatabase(page);
  return pageId;
}
catch (e: Http400Exception) {
  return handleNotFound(e);
}
catch (e: Http500Exception) {
  return handleServerError(e);
}

This kind of error-subclassing pattern is really common in promise driven code but very rare in callback code because throwing is so discouraged.

The fact that operational vs programmer error differentiation is even possible in node is an incidental side effect of this requirement that async callbacks never throw and therefore normal error handling must be done differently. This kind of difference cannot be detected in a synchronous try..catch block for example.

Promises treat errors in a way more similar to a try..catch, and therefore their error propagation mechanism and error handling conventions are different from callbacks, but the capabilities still exist.

[Qard]

-1 for recover object as an argument. It conflicts with optional object arguments, which the exact fs.readFile(...) call that has been used in examples supports. Having two optional object-type arguments in a row would just be asking for trouble.

+0 on the promise.recover({ ... }) method though. As suggested already, I just don't think the try/catch approach will work adequately for post-mortem until checked/pattern-matched exceptions are a thing.

Personally, I don't actually care much about the post-mortem abort expectation. If you care so much about runtime safety, why would you be writing JS? I'm not going to argue that point though--I recognize I have a strong opinion that some don't agree with there.

[chrisdickinson]

@qard: At present, the recovery object is mixed in with the options object — it's always the last object argument.

@phpnode: Yep — I do this as well in my bluebird-based code. However, this isn't acceptable for error symposium users, since they want to separate "known failure state" from "unknown failure states", and they'd prefer unknown failure states be easy to locate when they emerge.

[DonutEspresso]

I've been peripherally involved in some of these discussions, so I'll continue to provide some input so long as it's useful. To start, I agree with @spion that expected/unexpected is a good way to frame the discussion. I'll try and summarize in a few short bullet points what my typical game plan is, which is likely similar for some of the other systems oriented folks:

• Use errbacks as the primary channel for communicating expected errors. If you're using an API that throws underneath the hood (JSON.parse), localize your try/catch and return the error in an errback. If you're ever handed an Error object, that's an expected error, so it must be handled.
• When it comes to unexpected errors, most of them are usually automatically thrown, so fail fast and fail hard (abort)
• The action of aborting should generate core dumps useful for post mortem debugging
• Complete the loop by going back and fixing the source of the error.
• Because all expected errors are communicated via Error objects, anything that throws is assumed to be catastrophic (abort). Whether that's a typo, a null ref, or an explicit throw in code you own.

By adhering to this strategy, you effectively have two "channels" of communicating errors. You have an errback channel for expected errors, and an throw channel reserved for fail fast unexpected errors. IMHO the crux of the disagreement is almost certainly this: when possible, I actively avoid the use of throw/try/catch as a channel for communicating expected errors. I do so primarily because:

1) catch as it exists doesn't allow me to safely differentiate expected from unexpected errors. instanceof isn't foolproof, and fails under various scenarios. Same with duck typing. And even if pattern matching lands, checking on e if TypeError isn't always semantically correct because APIs could be returning TypeErrors as expected errors. Unfortunately, JS's lack of real types hurts here. 2) unwinding the stack only to have to determine if we need to rethrow is a non-starter for post-mortem

To be super clear about this - my hesitation around using catch apply even outside the context of promises. I also want to note that the construct we've created of having two channels to communicate these errors is also a necessary evil (this is where the checked/unchecked errors came into being in the symposium threads). It seems unlikely that we'd go through all this trouble were it easier to discern errors with more confidence. JS's catch is spiritually identical to Java's catch (Throwable e).

TL;DR, I think we all agree on expected/unexpected errors existing, and needing to differentiate them. Promise users do that using .catch(), non-Promise users do it by never putting expected errors into the throw channel. Camps may disagree on how to handle unexpected errors (abort or not), but I think that's an orthogonal concern to this discussion.

I'm starting to think that the discussion really is around how we can make Promises compatible with post mortem debugging (if possible). The context of most of these threads is around trying to figure out how/when to abort process on unhandled rejections without breaking the expected behavior of Promises, and using flags to enable/disable those breaking behaviors. I might posit that if only for supporting the use case of universal JS, using flags has some implications in terms of surprising behavior depending on what side of the wire that code is run on.

[petkaantonov]

@DonutEspresso

You are using unexpected/expected errors to mean operational/programmer errors. The distinction, and what @spion means, is supposed to be that whether an error is unexpected/expected is decided by the consumer.

Having two error handling channels doesn't add to this because the producer is the one choosing the channel. You are just adding extra work for the consumer which now has to handle both channels to see what they expect and not. You cannot have one channel for unexpected and one for expected because the producer is choosing the channel while the consumer is the one who must decide what is expected and what is unexpected.

Here's a silly example but you really should read spion's example about image cropping and transformations, they are more interesting

// If this throws, it is clearly unexpected
port.open(99999); 

var thePort = prompt("which port do you want to open?");
try {
    // If this throws it is clearly expected
    port.open(thePort);
} catch (e) {
    alert("Sorry, the port " + thePort + " couldn't be opened because: " + e.message)
}

This should make it clear that the port.open implementation cannot in any way decide what is expected or unexpected.

2) unwinding the stack only to have to determine if we need to rethrow is a non-starter for post-mortem

Not with proper tooling

[DonutEspresso]

@petkaantonov I don't think we're saying different things.

If I were being dogmatic about this, I'd say that the problem is partially because port.open() (and likewise JSON.parse and its ilk) decided to throw an expected error instead of returning me an Error object. As you said, it's now forced me to look for errors in two different places based on whether the operation was asynchronous. It's why stuff like safe-json-parse exists.

In an errback based world, given that functions are the lowest common dominator, having independent channels is the primary (only?) path forward to normalizing those errors. Force everyone, regardless of asynchrony, to use errbacks. I'm not saying it's the right solution, or that they even apply to promises, just stating that this is the current state of the world. We've settled on using return codes to normalize error handling.

Promises take a vastly different approach by streamlining everything into a single channel and handling them via catch. IMHO, which approach you take is completely driven by how comfortable you are with the trade offs re: post mortem and fidelity in differentiating errors via catch.

Regarding @spion's crop transform, sure, it is entirely possible a second transform can become invalid after the first is applied. That is most definitely an expected error in my view, because it's something you should have been thinking about when you designed the crop transform class. By not handling an possible expected error you turned it into an unexpected error. Differentiating expected from unexpected is not just about being defensive against bad input.

It would be lovely if we could fix the post mortem issues for promises. Unfortunately, whatever solution we can come up with there doesn't help with regular try/catch.

[spion]

@DonutEspresso as demonstrated even if you are tediously defensive (as in the first example), the function applyTransforms there breaks the ability to be defensive. You can no longer be defensive without solving the halting problem - i.e. you can't know in advance if the list of specified transforms is a valid argument without applying them. You must return Result<T> instead, which is pretty much the sync version of promises in a language that has no exceptions.

regarding sync try-catch, one of the possible alternative solution in #8 may be able to offer enough flexibility to do PM debugging even in the face of try-catch. To be honest I don't know if its even remotely doable, but it seems worth giving it a try.

[DonutEspresso]

@spion Yes, I would have done something similar to what you proposed. I also don't think it's unreasonable in this situation to use a localized try/catch (YMMV depending on who you ask) within your internal implementation. Presumably, in such scenarios it's also much more clear whether or not the code in your try block is a recoverable scenario. The whole thing about avoiding throw/try/catch is really around using it to communicate error scenarios across API boundaries.

@chrisdickinson Sorry for derailing this thread! Some of this is probably good info for the post mortem doc.

Re: the recovery object being proposed here - after thinking about it some more I think I'd echo some of the other concerns in terms of interoperability. If I wanted to write an abstraction top of fs, I for sure need to mimic the recovery pattern in my wrapper. I'm not a heavy promise user, but I would imagine that it might be terribly confusing for new comers to have to think about how/why to use two completely different error handling semantics within the same abstraction.

[chrisdickinson]

We're verging into "invalidating the use case" territory a bit here. It's a bit hard to disentangle the needs of the error symposium from the needs of the post mortem group, but I'll give it a shot: My impression is that error symposium users wish to avoid APIs that require the following:

try {
  var val = JSON.parse(input)
} catch (err) {
  // bad json!
}

As @DonutEspresso noted, this is where we see things like safe-json-parse pop up. The worry I hear expressed by the error symposium is that, as async/await lands, absent action on our part to accommodate their needs, all Node Promise APIs will be written as:

try {
  var val = await fs.readFilePromise(input)
} catch (err) {
  // could be either bad input or bad state on disk.
}

How do we accommodate users who wish to differentiate between bad input and bad state? Especially, how do we accommodate users who wish to differentiate and are using post mortem tools?

It seems like there's pretty unanimous agreement that the recovery object route doesn't satisfy everyone's needs. Does --abort-on-sync-rejection? Under that proposed flag, the example above would abort at top of stack if input was bad, proceed to catch (err) on operational errors, or continue normally if no errors were encountered. It's analogous to the Checked/Unchecked error split proposed earlier in that it causes certain kinds of errors to abort immediately. Would a program running under such a flag encounter many difficulties in the ecosystem today? How common is synchronous rejection?

@DonutEspresso Not at all — thank you for your continued input!

[chrisdickinson]

@rvagg:

Thank you for contributing this perspective.

I'd like to caution on treating this as a concern that can be isolated to "the error symposium participants" and "the post mortem WG" as it risks suggesting this addressing the needs of a very small interest group, which it's not.

An excellent admonition. I've been referring to the concerns brought by the groups by the names of groups themselves mostly for the sake of parsimony — I agree that it's important to keep in mind that the concerns that they are voicing are reflective of a larger group of users. Do you think documentation at README.md level would help address this concern? Would you suggest splitting the concerns-<group> labels out?

However, if we can address the concerns of people who see Promises as detrimental to discovering and addressing programmer errors then we might end up in a place where even they can be OK with their teams and companies they consult to using Promises because there's an escape hatch of some kind to bring them in line with their particular philosophy about how to deploy, debug and improve Node.js applications.

How does --abort-on-synchronous-rejection strike you in terms of providing this escape hatch? In particular, it would mean that the following program, run under the flag, would immediately abort with a useful core in the case of a bad filename input:

module.exports = function (filename) {
  return fs.readFilePromise(filename)
}

[phpnode]

@rvagg I feel like you made a lot of sweeping statements in that post that are pretty easy to disagree with, and your own dislike of promises is showing through. You are effectively saying that promises make node.js programs harder to debug, when the vast majority of promise users will say the complete opposite.

Promises are not something you should automatically jump to if your coders are writing error-prone code

Again, I would totally disagree with this. In my experience, Promises make it much easier to write robust code with proper error handling mechanisms. Programmer errors reveal themselves in just the same way as they do in callback driven code.

The key point though, is that callbacks and promises are different and so they have different semantics and their users have different expectations.

[spion]

Startup is cheap with Node.js and every Enterprise level deployment uses a cluster approach of some kind so the impact is relatively minimal on users.

This has not been true in our experience. Startup with Node is not nearly cheap enough, and there is always the tendency of the main (especially web serving) process to quickly blow up above the 1 second startup time, most of it spent synchronously loading thousands of modules from disk.

The impact is also not minimal because a single process serves thousands of users concurrently and all of them will lose their connection and may lose or corrupt their work when it crashes (depending on the guarantees of the storage layer)

But I digress.

Arguing about whether fail-fast or resilient-in-the-face-of-errors is the correct approach for Node.js will get us exactly nowhere.

This issue as stated at the top is not clear. It reads to me as "implement current node philosophy about error handling in promises". Which is a paradox and will definitely lead to the argument we want to avoid.

Post-mortem debugging on the other hand is a clearly stated problem.

[benjamingr]

First of all thanks for dropping by @rvagg and discussing this with us. We'd really like to have you on board for the discussions so we can reach consensus.

Operations wants to have good insight into what went wrong so they can understand the scope of the problems and feed it to the right person to get it addressed, this is partly where postmortem debugging comes in (although it's actually pretty rare to see the kinds of advanced practices that Netflix and Uber use).

Right, promises change the thought-model. Bugs come with a lot more context since they are from composed chains (since a promise chain doesn't live in isolation). You typically get a ton of context on programmer errors and can find where things happened very quickly.

But at the programmer level you get a defensiveness that can lead to overcompensation, often resulting in error-minimisation or error-hiding practices like heavy use of try/catch or adopting Promises (so the errors go away completely).

Errors don't go away with promises. In fact - they propagate. If with callbacks you don't have to check if(err) at every step of the way - promises mandate that. Errors are no longer suppressed by default unless checked. If anything I think promises mitigate this problem rather than enhance it.

Of course neither of these is good practice for dealing with bad code and experts usually come in asking to wind back a lot of the safety mechanisms to expose the poor programming.

This is precisely why we added unhandledRejection - so promises never silence errors - even synchronous ones.

This is where fail-fast comes in as a mantra that's generally promoted with Node.js. Fail quickly when something unexpected happens, log or dump what you can in order to figure out what happened and restart.

The mental shift with promises is that you can get most of the useful parts of the debugging info and recover so you don't have to restart until you have a fix to apply. The server stays in consistent state at every step of the way since cleanup is performed as the stack is unwound.

Startup is cheap with Node.js and every Enterprise level deployment uses a cluster approach of some kind so the impact is relatively minimal on users.

This is something I don't understand - let's say you have a programmer error in your deployment. You have a problem where at /users/foo?bar=15 a TypeError occurs (for a very rare path). You "fail fast" and crash the server and dump core. Now let's say your startup is lightening fast and takes just 4 seconds for this big project - you also have a very strong machine for your service running 8 workers.

If I send you 2 requests every second (which is trivial) I can effectively cause a DoS attack on your server. What post mortem recommend (at https://github.com/nodejs/post-mortem/issues/18) is to actually leave your server in inconsistent state and just hope for the best.

This is not contrived, @Raynos describes Uber having that problem here: https://github.com/nodejs/post-mortem/issues/18 and I've personally seen it when consulting in several other big companies.

simply that Promises are not something you should automatically jump to if your coders are writing error-prone code

Obviously. Promises are not a magic bullet that somehow "fixes async" or causes you to "not have errors". They do however in my experience simplify my workflow greatly. What pattern you'd use for asynchronous coding is opinionated and while we can have a lot of fun debating it is not our place to make that choice for users and should offer users what they need to make an educated decision - making the lives of both people who use and avoid promises easier.

In fact, I suspect that the clash of philosophies over error handling is one of the biggest issues preventing a more nuanced discussion about the merits of Promises vs pure callbacks.

We'd really just want to see both coding styles supported in Node.

However, if we can address the concerns of people who see Promises as detrimental to discovering and addressing programmer errors then we might end up in a place where even they can be OK with their teams and companies they consult to using Promises because there's an escape hatch of some kind to bring them in line with their particular philosophy about how to deploy, debug and improve Node.js applications.

This is precisely why we introduced unhandledRejection in the first place. You can just add a e => { throw e; } and have thrown errors fail fast with promises. This is the escape hatch and you can in fact use that philosophy.

[rvagg]

@phpnode and others, this defensiveness has to stop if you want to make progress here, you're not incentivising people with concerns to interact with this group and will simply entrench the divide. Please take my feedback as an actual perspective that exists and that you're going to have to engage with in order to break out of a WG and into a concrete implementation. I certainly don't have the energy to get bogged down in argument and don't appreciate you viewing me as someone with a faulty view of Promises that needs to be corrected, and I'm betting a lot of other collaborators are in the same boat.

[benjamingr]

@rvagg

Please take my feedback as an actual perspective that exists and that you're going to have to engage with in order to break out of a WG and into a concrete implementation.

If you do not think that I have done that let me know. I believe I have. I definitely do value and appreciate your feedback here. I do however want to discuss it - there are gaps in my understanding of how you deploy code and what your requirements are and debating these things is important.

This working group is not only about "getting promises into core". It's also about making sure your workflows are not broken or ignored. Currently we're at a suboptimal solution where you're uneasy consuming code using promises (and not just writing that) - I don't see changing your perspective as a goal, I do want to understand it and make sure that the current things that are problematic for you are addressed like AsyncWrap with the microtask queue, post mortem debugging concerns and so on.

[chrisdickinson]

Just a quick reminder that the topic of this discussion is scoped to differentiating operational errors from programmer errors. I believe @rvagg has reinforced the position that there are folks that really want some way of escaping from promise error propagation for truly erroneous, unexpected errors; we should take his (and @DonutEspresso's) use cases as valid and avoid credential-checking.

At present the only proposed option that is thus far unopposed is --abort-on-sync-rejection. Can I ask for a quick read on reactions to --abort-on-sync-rejection? Are there immediate problems folks see with it? Does it go too far, or not far enough?

[boneskull]

I'm in agreement with @spion 's assessment:

This issue as stated at the top is not clear. It reads to me as "implement current node philosophy about error handling in promises". Which is a paradox and will definitely lead to the argument we want to avoid.

I've stared at this issue for 45 minutes and cannot come to any other conclusion.

--abort-on-sync-rejection doesn't go far enough. Synchronous rejection this way seems rare, as stated. If you want to fail fast, you still have to do extra work. Imagine a terrible, dystopian world in which this code was commonplace:

func()
  .catch(panic)
  .then(anotherFunc)
  .catch(panic)
  .then(yetAnotherFunc)
  .catch(panic);

function panic(err) {
  // check err type here via some means
  new Promise((_, reject) => {
    reject(new Error(err));
  });
}

If you want to fail fast, Promises aren't your tool. Thus, the paradox as @spion writes. Maybe this should be closed?

[chrisdickinson]

@boneskull: To make sure we're on the same page: assuming func() is a Node Promise API-provided function, with the current proposal --abort-on-sync-rejection should abort the program during the func() call, skipping all subsequent .catch- and .then-installed handlers.

[phpnode]

@chrisdickinson this would catch so few cases it doesn't seem worthwhile. Programmer errors can obviously happen in promise handlers too.

If the need to differentiate here is important in promise code (and like others in this thread, I am not at all convinced), this solution is unlikely to satisfy the people who are asking for it.

[chrisdickinson]

@phpnode Notably this would apply to Node core Promise API's used inside of the handlers of promise chains as well. For example: Promise.resolve().then(() => fs.readFilePromise('data\0000')), if run with the flag, would abort the program.

[phpnode]

@chrisdickinson ok but that only covers core APIs, it doesn't work for the general case and it seems like that is what people who are advocating for this are looking for. It would be interesting to know whether @rvagg and others would be happy with your proposal.

[petkaantonov]

@DonutEspresso

I don't think we're saying different things.

I think we are, I couldn't find the words "producer" and "consumer" in your comment. The terms unexpected/expected and programmer/operational error are actually arbitrary and their definitions could be flipped. The distinction is whether the error category is decided by consumer or producer. You cannot deduce this easily from the terms, because "programmer/operational error" dichotomy could mean consumer decision if it didn't have historical producer decision baggage.

If I were being dogmatic about this, I'd say that the problem is partially because port.open() (and likewise JSON.parse and its ilk) decided to throw an expected error instead of returning me an Error object. As you said, it's now forced me to look for errors in two different places based on whether the operation was asynchronous. It's why stuff like safe-json-parse exists.

You are repeating what you said in previous comment. The problem is not that the producer made the wrong decision, the problem is that the producer made a decision at all. I can easily produce example where safe-json-parse is wrong:

var config = safeJsonParse(criticalConfigurationFileWithoutWhichTheServerCannotEvenStart)

Clearly any error in the JSON is unexpected and now safeJsonParse made the wrong decision to decide that the SyntaxError is expected.

You could say that the consumer can decide to use JSON.parse when invalid JSON is unexpected and safe-json-parse when invalid JSON is expected. However this is only because there is just 1 possible error. If there was just 3 different errors, you would already need 8 different APIs to cover all the possible combinations.

[benjamingr]

In my opinion --abort-on-sync-rejection strikes a good balance and I would be happy to use it since I never attach rejection handlers asynchronously anyway.

That said, others are objected to defaulting to aborts on unhandledRejection or limiting package authors to always add catch handlers synchronously. The problem is that the person who argued the hardest against unhandledRejection indicating a problem all of the time is @domenic and I'm afraid he would not be willing to participate in these discussions.

@petkaantonov had a strong opinion about --abort-on-sync-rejection yesterday maybe he'll share.

[benjamingr]

@phpnode

@chrisdickinson ok but that only covers core APIs,

Please see https://gist.github.com/benjamingr/3fb97eda723ff2a030d1 that explains this further.

[petkaantonov]

I don't have strong opinions about opt-in behavior

[phpnode]

@benjamingr thanks, I hadn't seen that.

[benjamingr]

@petkaantonov in particular I'm talking about the possibility of unhandledRejection aborting would be opt out and not opt in that @chrisdickinson raised.

[petkaantonov]

Are you flipping definitions of opt-out and opt-in? If there is a flag to enable it, it's an opt-in :P

had a strong opinion about --abort-on-sync-rejection yesterday maybe he'll share

I cannot have a strong opinion about this because it's a flag and therefore an opt-in. You could have a --self-destruct-in=15 flag and I wouldn't mind.

[petkaantonov]

I'm pretty sure that others would have lots of fun stories to see the kinds of horrors involved in mixed-skill teams at large companies who are only beginning to come to grips with Node.js. I'm also pretty sure that a lot of parties involved in training and consulting in these environments would not be promoting the use of Promises as a solution to any of these specific problems being encountered at the beginning of a Node.js journey (this is not about Promises as a pattern, simply that Promises are not something you should automatically jump to if your coders are writing error-prone code).

I could make essentially the same post you have made based on my training and consulting experience at Big Enterprise (Nokia) but with completely opposing conclusions. But I won't because trading anecdotes is not useful (try this diet it cured my cancer). If you want to play the "callbacks are better" argument please back it with peer-reviewed academic research. Otherwise you need to accept that both are in widespread usage by both big and small companies and both needs deserve same level of consideration, it isn't a competition who can throw in more anecdotes.

[benjamingr]

@petkaantonov I'm referring to @chrisdickinson 's earlier suggestion of making unhandledRejection abort if no handler is installed.

[petkaantonov]

That is in theory very backwards incompatible but we should have some idea how much will break and how badly before doing it.

[petkaantonov]

To actually have a good idea about it we would first need to ship with analytics that counts how many aborts would have been caused that wouldn't have been caused otherwise.

[spion]

Ok I have a clarifying question. What does --abort-on-sync-rejection achieve other than enable post-mortem debugging?

[benjamingr]

@spion enabling post-mortem debugging is precisely what it does. As far as I'm aware there are no other reasons for the flag.

[balupton]

Regarding handling operational errors (currently planned via .catch but also some suggesting for throwing for try...catch) vs programmer errors (currently implemented as throwing errors). Why not have programming errors be of their own error type (say EINVALID), that can be passed to the callback/promise — this would ensure consistency in the methodologies of handling errors, as one can use the same syntax for filtering all failures — rather than having to use both .catch for operational errors and then couple that with try...catch for programmer errors (or whatever other additional channels are being discussed here), which is a bit of a pain. The argument for why not doing this is generally to distinguish between the two error types, but that is void as the introduction of a new error type class for such errors accomplishes such distinction.

This would introduce some oddities while at the same time removing others, however it seems novel as all the error handling proposals also have oddities regarding the same points, and is the only one that utilises the same channel (.catch) while maintaining the benefits (differentiating between programmer errors and operational errors).

Proposals in this comment:

1. Make current thrown errors have their own error type so they can be easily identified — throws and try catch remains
2. Make current thrown errors be passed to callback/promise instead (optional) so they can be easily and consistently handled — try catch mostly goes away

[phpnode]

One thing I haven't seen mentioned here but might be relevant - Promises can be rejected in two ways:

throw new Error('NO!');

or

return Promise.reject(new Error('NO!'));

Bluebird offers a way to differentiate between these. Maybe we could do the same in node and optionally treat the former differently from the latter.

[petkaantonov]

@phpnode Bluebird doesn't offer a way to differentiate between those. It only offers that when you translate from callbacks to promises using promisify/promisifyAll. Otherwise rejections and throws are the same thing and should be the same thing, they are chosen by the producer and thus cannot make the proper choice for consumers.

[phpnode]

@petkaantonov ah I see, I never used that feature, just remember reading about it in the docs.

[petkaantonov]

Yea IIRC it was in 1.x or 2.x.

[groundwater]

@spion I think @groundwater proves the exception to the assertion that promise users do not find differentiating these errors necessary. He is a user of promises via async/await, and finds it necessary to differentiate "I called the API incorrectly" from "this is a recoverable state of the program."

@chrisdickinson I don't think I'm the exception. Anecdotally I haven't encountered a person who, in the course of debugging a real example, has wanted errors swallowed as indiscriminately as they are by the Promise API. This has held across browser and server projects alike.

Usually people switch to Bluebird in order to get unhandled rejection errors.

[petkaantonov]

@groundwater Unhandled rejection errors are available in basically all implementations and environments. They are available in browser native, node native, bluebird, q, rsvp, when. Clearly promises swallowing errors and bluebird being the only solution is just an unsubstantiated rumor.