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.