Add Recovery Method

[groundwater]

@chrisdickinson @zkat

Your comment is a pretty good start. Figure we can go from here.

[chrisdickinson]

LGTM! Here's an initial spike.

[retrohacker]

I'm a fan of this approach.

It offers a pretty decent way to handle errors in a promise chain . As opposed to fully rejecting a promise and making the assumption no other function in the chain can continue, it allows you to pass the error object down the promise chain so that further functions can determine whether or not they can recover. Then the error can be handled sanely outside the chain.

[groundwater]

A question and a comment @chrisdickinson

1. Like I said in Update README.md #5 we should only put operational errors in the recovery object. This includes avoiding matching ReferenceError as well as fs.statFileAsync with isFile, isDir etc.
2. Is there a regular way of mapping from the error name to the recovery identifier? e.g. EISDIR(){...} with EISDIRError etc.?

[benjamingr]

This is a nice idea but I think we could solve this without adding an actual argument and conflating the entire core API.

I remind you that promises are designed to be subclassed in ES2015. We can extend promises (in a standard compliant way) with a recover method that calls catch internally and performs this filtering.

This means you can't substitute native promises with bluebird - but I don't think that was a goal anyway.

[groundwater]

Hey @benjamingr do you have an example API you're thinking of? I'm also not sure what you mean by "conflating the core API".

It sounds like you're requesting something like

let file = await fs.getFileAsync('dne').recover('EISDIR', () => null)
if (file === null) {
  return
}

[benjamingr]

I'm on mobile, so sorry for being a little unclear and not posting code - I hate it when other people do that.

I like the object API you have. Basically, recover would take an object and behave exactly like in this proposal. The only difference is that it'd be chained off rather than a function argument.

By conflating I mean that as an extra argument each core method would have to be separately aware of it - in addition users that want to use it in their code would have to roll it manually (not a lot of code to write but no code is better than code).

It could handle common Node errors like enoent and such (again, exactly like in the parameter variant).

We'd be amendable to adding it to bluebird if core adopts it. I think it's really ergonomic.

[benjamingr]

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

[groundwater]

@benjamingr thank you for explaining. To clarify, do you see the following as recovering or failing:

fs.getFileAsync('dne').then(() => fs.getFileAsync('dne')).recover({ ENOENT() {...} })

Specifically does the "error" fall through to the final recover handler?

[chrisdickinson]

@groundwater:

Like I said in #5 we should only put operational errors in the recovery object. This includes avoiding matching ReferenceError as well as fs.statFileAsync with isFile, isDir etc.

Yep — that is the case with the reference implementation. The only code that can reject the promise is located here — which is to say, only the wrapped callback API (the fs.readFile to fs.readFilePromise) may reject the promise. The only rejections those callback API functions should immediately throw are programmer errors due to invalid parameters (so programmer errors in async/await still throw as they do in the callback API, and are not subject to the recovery object.) One nice side effect of this is that programmer errors are immediately handed to the unhandledRejection handler, which if I understand @misterdjules' work correctly, could be made to abort (as desired) with a flag.

Is there a regular way of mapping from the error name to the recovery identifier? e.g. EISDIR(){...} with EISDIRError etc.?

Node should generate all operational errors with .code attributes mapped to the man 2 intro error list. I'll have to make sure that this is the case, but it seems like an assumption that is safe to make. Right now I'm using the .code identifier to map EISDIR errors to the {EISDIR(){}} handler.

[chrisdickinson]

I'd caution against the .recover-style API, for a few reasons:

1. 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.
2. 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.)
3. 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.

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.

[DonutEspresso]

I'm just getting caught up on the many thread(s) again, so forgive me if I've missed something. I know there's been a lot of cross posting between this thread and some of the post-mortem threads.

I'll have to think about this a bit more, but on first impression - kinda liking what I see here. I might also suggest a potential catch all for all operational errors. I'd say it's the rare case where I make the differentiation between error types when fs.readFile fails, just that I know it failed operationally, as opposed to me making some typo in the function invoking it.

@chrisdickinson I think you may have touched on this, but if my understanding is correct, this solution would be specific to the promisified core APIs? Unless everyone is using verror or restify-errors to create error objects, userland created Error objects won't consistently have a code property. More interestingly, how do you folks envision this recovery pattern interacting with userland promise returning APIs that may use/wrap the core APIs? Seems to me like it would necessarily be a turtles all the way down problem - everything in the chain above you must use this recovery pattern or else post-mortem benefits could be lost somewhere along the way.

[chrisdickinson]

@DonutEspresso Sorry for the delay in response! Would you accept trap(err) as a "catch-all" handler name?

@chrisdickinson I think you may have touched on this, but if my understanding is correct, this solution would be specific to the promisified core APIs? Unless everyone is using verror or restify-errors to create error objects, userland created Error objects won't consistently have a code property. More interestingly, how do you folks envision this recovery pattern interacting with userland promise returning APIs that may use/wrap the core APIs? Seems to me like it would necessarily be a turtles all the way down problem - everything in the chain above you must use this recovery pattern or else post-mortem benefits could be lost somewhere along the way.

This pattern, as proposed, is specific to the Node API. I'm not sure we can solve the behavior of existing packages immediately — my suspicion, though, is that if this pattern is introduced by core it will be emulated by the various shims, and more broadly emulated by the ecosystem.

I am curious: as an alternative, if we added --abort-on-sync-rejection, any programmer errors passed to Promise-based Node APIs would crash immediately with a core. Would that be a more reasonable way to address the problem of separating operational and programmer errors?

[benjamingr]

I am curious: as an alternative, if we added --abort-on-sync-rejection, any programmer errors passed to Promise-based Node APIs would crash immediately with a core. Would that be a more reasonable way to address the problem of separating operational and programmer errors?

Errors are thrown synchronously inside promise chains all the time. That wouldn't work and would break the ecosystem.

[chrisdickinson]

@benjamingr:

Errors are thrown synchronously inside promise chains all the time. That wouldn't work and would break the ecosystem.

Ah! I'm not talking about somePromiseMethod().then(() => { throw new Error() }). I'm talking specifically about:

new Promise((resolve, reject) => {
  throw new Error() // or reject(new Error())
})

Which (should be) far less common.

[chrisdickinson]

(This could be paired with patching the Promise.reject helper method to reject on next tick, which should be a fairly innocuous change)

[benjamingr]

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.

---

@groundwater

fs.getFileAsync('dne').then(() => fs.getFileAsync('dne')).recover({ ENOENT() {...} })

Specifically does the "error" fall through to the final recover handler?

Yes, it does. This allows chaining and nesting which the current proposal does not. Instead of a "magical" parameter we just extend the promise interface (in a spec compliant subclass way) and add a recover method.

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));

[chrisdickinson]

We've moved the conversation here.

[groundwater]

@benjamingr thank you for explaining your idea further.

Unfortunately I think your example is a counter-example, in that it uses .catch which defeats the ability to do post-mortem. Given that this document is about exploring possibilities, I'm not saying we can't include this idea, but it would be beneficial if the solution preserved post-mortem, meaning the VM needs to know it should crash at the time of a throw without evaluating any further JS code.

I think the central issue is that, by passing the recovery object as a parameter, the API can immediately decide if an error is recoverable or not. I am unclear with the fall-through behavior how an API can decide if a handler is in play or not. The recovery information must be available directly to the API, meaning the API must know if the handler exists or not.

It would be worth finding the boundaries of this solution regardless, because I bet it will come up again. Having a clear example explaining the pros/cons is probably worth the time (even if it doesn't make it into core).

[benjamingr]

Unfortunately I think your example is a counter-example, in that it uses .catch which defeats the ability to do post-mortem.

Why? Just having something that can be described with a .catch doesn't mean we can't do post-mortem at all.

Remember, the actual promise code executes after the handler has been attached since promise handlers always execute asynchronously. This would be the same with a .catch or a last argument.

[groundwater]

Just having something that can be described with a .catch doesn't mean we can't do post-mortem at all.

Having a .catch forces the stack to unwind before deciding if the error should terminate the program or not. A post-mortem requirement has been that programs terminate with the full program stack in place at the time the error is thrown.

This isn't to say a .resolve method requires a .catch but your example would not meet the post-mortem requirements.

[benjamingr]

I was under the impression that if the object is not present or it is present but not handling the error than the rejection would propagate normally outside. How would it help post-mortem?

[groundwater]

@benjamingr in the above example, attaching a recovery object will run this.catch which catches all errors, not just the errors we expect to see. Unfortunately re-rejecting or re-throwing at this point does not address the post-mortem concern, because you want the program to halt at the place of the original throw.

The only working promise "compromise" so far with post-mortem is that you cannot use .catch. i.e. all rejections must terminate 100% of the time. The recovery object is there to avoid rejecting when you encounter an expected (operational) error. This will let all other rejections bubble up and cause termination (if you want post-mortem).

If you prefer .catch and decide that type of control flow is worth the tradeoff, that is still possible.

[benjamingr]

@groundwater oh, I think I understand where the miscommunication is stemming from. Just because I can implement it with .catch doesn't mean that Node should implement it that way.

As far as post-mortem is concerned this can be implemented without a .catch and not catch all errors. Then if no catch handlers are attached post-mortem works fine. This requires C++ code changes and to write .recover in C++ code (and not JS).

Note though, that there is an implicit try/catch around things awaited in async functions and terminating on rejections that might be handled. So those assumptions all fail in async/await code.

We would need V8 to expose whether or not the enclosing async function is running the code contains a try/catch block (what @spion mentioned).

[groundwater]

@benjamingr looks like @chrisdickinson and I remain unconvinced this approach is the right investment. I don't want to block you from exploring it, but I don't see enough evidence to alter this PR significantly.

I do think we should add a link to your comment into the document for anyone wishing to explore this type of API.

If you'd like to PR a new section adding this approach, I would happily except it instead of just linking to your comment.

[benjamingr]

@groundwater a last-parameter-object is currently blocked on the possibility cancellation tokens will be introduced in the future.

I'm not suggesting we add .recover - I'm just saying it's a proposal worth considering.

[chrisdickinson]

Cancellation tokens? To the Node API or to Promises? If the latter, then it shouldn't affect the recovery object, which never touches the promise API.

[benjamingr]

If cancellation tokens will be added they'll be passed as a last parameter to the function. So - the former. This is some nice reading.

[chrisdickinson]

@benjamingr Interesting — I'll give that thread a more thorough read this evening. First impression is: this means that cancelation tokens are passed to async-branded functions, not to promises themselves? That seems like it might be a bit of an overreach on the part of the spec, since it means that the spec would start dictating how Node presented its API.