domenic / promises-unwrapping

The ES6 promises spec, as per September 2013 TC39 meeting
1.24k stars 95 forks source link

Promise.prototype.done #19

Closed medikoo closed 11 years ago

medikoo commented 11 years ago

With current design, the only way to process resolved value and be sure that unhandled exceptions are exposed, is via following

promise.then(function (value) {
  setTimeout(function () {
    // Process value
   }, 0);
}, function (err) {
  setTimeout(function () {
    // Handle error
   }, 0);
});

Which is poor solution for many reasons:

In many promise implementations, there's already promise.done, which doesn't create any new promise, and invokes callbacks naturally (outside of try/catch clause).

As done cannot be implemented on top of available API, I think it's very important to include it in spec.

As side note, why do Promise.prototype.catch is specified? It can be very easily configured on top of the rest of the API, and it's usability is low.

medikoo commented 10 years ago

You can just put .done() since you can just add it to prototype (aren't they a wonderful thing).

That's not possible. You can cleanly implement then over done but not the other way. You can try to mimic done behavior on top of then as you suggest, but that's far from perfect, it's a poor man's solution.

ForbesLindesay commented 10 years ago

Yes, user's can add their own .done method, no that does not mean it should not be in the spec. It has already been well proven in user space. We know it is useful, so it is ready to be specified and then implemented.

jakearchibald commented 10 years ago

Here's the V8 ticket for logging unhandled rejections https://code.google.com/p/v8/issues/detail?id=3093.

The current plan is to console.error rejections to promises that don't have a rejection handler on them specifically or further down the chain.

So this will console.error:

Promise.reject(Error("BROKE"));

This will not:

Promise.reject(Error("BROKE")).catch(function() {
  return "Nah, it's ok";
});

However, this will console.error (a false positive):

var p = Promise.reject(Error("BROKE"));

setTimeout(function() {
  p.catch(function() {
    return "Nah, it's ok";
  });
}, 500);

We're hoping the above case is rare enough.

medikoo commented 10 years ago

@jakearchibald there are cases like that, and they're not that rare (especially in node environment). So you'll definitely have false positives around. Such solution was already implemented by some libraries a while ago. RSVP.js has it and When.js (which added done to it's API recently) also. Check their issues, it was already proven as problematic.

Aside of general flaw that this solution has, other question is: What about other JS engines, and I'm not speaking of just browsers. Any implementation that would silent unhandled exceptions is doomed. I hope we all agree in that.

juandopazo commented 10 years ago

@jakearchibald Promise.reject logging errors becomes too noisy very fast. That particular case should not log errors. This should though var p = new Promise((resolve, reject) => reject(foo));.

In YUI's implementation I'm specifically circumventing the unhandled errors logic in Promise.reject: https://github.com/juandopazo/yui-promise/blob/master/promise.js#L248-L259.

jakearchibald commented 10 years ago

Understood, will dig out the issues for other libraries and take them to the V8 team.

Sounds like debugging is in a really bad place.

So… what then?

I was convinced by this thread but it's feeling like done is the least-worst option. It's the developer saying "I'm not handling errors", which maps to not having try/catch, or try { ... } catch(e) { throw e; } in sync code.

domenic commented 10 years ago

The solution is pretty simple. A live list of currently-unhandled errors. One of the great advantages of browsers implementing promises is that they can accomplish this in their tooling, whereas until now library authors have had to use hacks like e.g. logging an array and counting on the developer tools to keep it live updating.

juandopazo commented 10 years ago

Honestly, to me done vs tools is a false dichotomy. We can do both. In A+ having the smallest API surface was crucial to have less to agree on. But ES6 is already introducing new methods, so I see no downside in also including done.

medikoo commented 10 years ago

done is seen to put too much responsibility on the developer

Anti done attitude takes it's source from "it would be beautiful to have just then" way of thinking, and not from: "I've tried done and it's too confusing to have it aside of then".

Let's ask people that rely on done in everyday work with promises, whether they find it problematic.

I've actually came from "only then" way of thinking to "done (as first choice), and then when transformation is needed" over year ago and it was just positive, never looked back.

It's also important to note that in most cases done is replacement for then and not addon that you have to and may forgot to add at the end of chain.

jakearchibald commented 10 years ago

The solution is pretty simple. A live list of currently-unhandled errors

But that's something the developer has to ask for, whereas a logged error is pushed at them. Also, this live list is even less accessible in a non-graphical environment like node.js.

The list thing (which I'd love to see in the style of a git graph) will happen, but is it enough?

spion commented 10 years ago

Here is a summary of solutions and their shortcomings:

Using .done()

Pros:

Cons:

Only the "Second" error will be reported.

Logging posibly unhandled rejections

Pros:

Cons:

Note: Indeed, some cases are easier to write with asynchronous error handler attachment. However, its likely that all of them can be rewritten - see rsvp No 196

Asynchronously attaching error handlers may lead to other hazards (what if the asynchronous operation that does the attaching fails? Very bad).

Live list of unhandled rejections

Pros:

Cons:

Special GC strategy

Pros:

Cons:

domenic commented 10 years ago

But that's something the developer has to ask for, whereas a logged error is pushed at them.

I don't understand this sentence. You open your developer tools and click "console": you see console.errors. You open your developer tools and click "promises": you see unhandled rejections. Probably you proxy some unhandled rejections over to the console, like you do XHRs---but you need to remove them from the console once they become handled.

stefanpenner commented 10 years ago

great summary @spion.

My gut feeling is recommended best practices would combine several strategies which would yield the best-possible outcome.

Maybe someone can find the silver bullet, but I fear the likelihood of that is low.

Another idea is to have a chrome extension that updates a visual counter of unhandled/pending counter in the main chrome (maybe they appear only after X ms). This would be similar to how we realize errors are happening in our apps. This of course is only useful for browsers, and browser users using the extensions.

stefanpenner commented 10 years ago

for reference for anyone who hasn't seen it, this is our current progress with related browser tooling: https://github.com/tildeio/ember-extension/pull/76

It's a start, lots more todo. If someone is interested I would love some extra bandwidth to work on having it a standalone extension (or maybe our devtools friends want to support this out-of-the-box)

medikoo commented 10 years ago

A lint tool is necessary to expose such mistakes

Not true. I use lint tools heavily, but never felt a need to use lint for that and never did.

Limited reporting when doing async error handling, e.g.

This code doesn't make much sense, and in its best interpretation behaves properly: it doesn't throw second error as fist (in order of processing) took precedence. Same when you process collection of promises and few of them become rejected, most (probably all) libraries expose only error of first promise that was rejected, and that's totally fine, it's expected and not perceived as an issue.

jakearchibald commented 10 years ago

Probably you proxy some unhandled rejections over to the console, like you do XHRs

Yes, we do this so the developer doesn't have to switch through every tab of devtools when something isn't working right. We hope to give them something in the console that gives them a clue, like we do with failed XHRs. We want to do the same with promises.

Also, this is what developers running v8 via the cmd line would see

but you need to remove them from the console once they become handled

…which we can't do on the cmd line.

We don't want the console to become really noisy, but ideally we want to tell the developer about unhandled exceptions, like we do with sync code.

domenic commented 10 years ago

Yes, the command-line debugging experience is not as obvious, because it's hard to represent a live list on the command line. But for web inspector, it's a no brainer.

stefanpenner commented 10 years ago

@jakearchibald the catch is, this isn't sync code. Likely some timeout tolerance to decrease the likely good. (GC hook would make it 100% fine, as long as your code didn't accidentally retain the promise forever, which is totally possible)

in light of that we still likely need a combo of a few strategies to give good solution here.

jakearchibald commented 10 years ago

I'm not arguing against the big graphical representation of promise state, we need that and something like that will go into devtools eventually.

However, we need something for cmd-line, and if we come up with something decent, that'll also go into devtools console.

(even in graphical devtools, I don't think removing things from the console is a great experience)

jakearchibald commented 10 years ago

Maybe the GC option is worth a shot. Although I'm worried about inconsistencies like some errors not being logged for difficult to understand reasons (cannot be GC'd due to still-in-scope or something like eval() in scope), or errors being logged out of order.

domenic commented 10 years ago

I know I've said this before, but it seems like it's time to say it again. I maintain strongly that the fundamental primitive here is a live list of unhandled rejections. It is fundamentally undecidable for a computer to know what that list "should" contain: should it be empty always; should it be allowed to contain things for only one tick; should it be allowed to contain things for ten seconds; should it be allowed to contain rejections specifically generated by certain proccesses... The only coherent strategy is to surface to the user the state of their system. Making a choice like "log immediately" or "log after one tick" is making an often-incorrect decision about how long things are allowed to remain in the live list.

Let us try an imprecise analogy. The list of outstanding network requests can contain connections that are kept open indefinitely, e.g. for long-polling purposes. The dev tools presents this as a live list of network requests, some of which get moved into "success" state, some of which get moved into "error" state, and some of which don't ever get moved out of the pending state, because they are used for long polling. Sometimes things stay in a pending state for a long time. But it would be crazy for the dev tools to decide "oh, this connection's been open long enough; I'll just log it as an error state and move on." Only devs can do that, by manually attaching timeouts (akin to devs manually attaching .catch(console.error)).

jakearchibald commented 10 years ago

That analogy is too imprecise. The browser logs failed network connections to the console when they fail. This gives the user a hint about the error without them going to the main network tab. I'm looking for something like that for promises that'll work when they're used in command line contexts such as node.js.

The live list is a great idea, I don't think anyone is arguing against it. However, we also need something for command-line cases, and that solution will also be useful in the devtools console.

domenic commented 10 years ago

I don't think a live list can be represented in a non-editable textual medium such as a traditional console log. As such, any solution that is console-only is simply insufficient for representing the potential error handling of a dynamic async system. Anything you do will be misleading.

spion commented 10 years ago

@medikoo a lint tool is necessary to expose such mistakes. You're claiming you don't make such mistakes - thats great! But others apparently continue to make them. Now we can argue whether the problem is education or not, but that wont change the fact that when a mistake is made, the user will not be warned of that mistake and instead will be greeted with the sound of silence!

Also, the second example does make sense. Imagine that both rejections are actually some asynchronous operations, but the second one attaches the handler. Lets take @stefanpenner 's example:

var facebook = facebookApi()
var twitter = twitterApi()

App.done(function(app){
  facebook.catch(function(error){
    app.reportFailure(error);
  }).done();

  twitter.catch(function(error){
    app.reportFailure(error);
  }).done();
});

Here if all the promises (App, facebook and twitter) fail, only App's failure will be reported.

medikoo commented 10 years ago

@spion Misuse of done or then doesn't fall into kind of mistakes that you solve with lint tools. Do you happen to use [].map instead of [].forEach by mistake? If you do it, it's only because you don't understand the API, and no lint tool would solve that.

In your example you misuse both catch and done, it should be written that way:


App.done(function(app){
  facebook.done(null, function(error){
    app.reportFailure(error);
  });

  twitter.done(null, function(error){
    app.reportFailure(error);
  });
});
stefanpenner commented 10 years ago

@medikoo you are incorrect.

spion commented 10 years ago

@medikoo if I write a chain of .maps, then remove the last one, then yeah it might happen that I forget to replace the last one with .forEach.

The solution isn't the bad part - the bad part is that your libary can't warn your users that they're doing it wrong when they do it wrong.

medikoo commented 10 years ago

@spion when you know done and treat it as first choice it's very unlikely to turn into corner of muted unhandled exception by mistake. Same as it's unlikely to leave map instead of forEach. I don't think I can remember such case on my side.

Anyway, even if, it totally can't be argument against done. It's rather argument that aside of done, it would be nice to have some aid in developer tools, which in such case is not a must have to work reliably with promises.

spion commented 10 years ago

You touched on another problem there in the example - done(null, handler); - maybe we also need .catchdone ? :)

medikoo commented 10 years ago

@spion I would rather remove catch from spec, it's rarely used (in implementations that have done) and it's just sugar over then, brings not much value.

spion commented 10 years ago

Its not rarely used. You often want a function that catches certain known error conditions.

Use case: A database query execution tool has a function that executes the query and prepares a result report. It also prepares a (different) result report when the query is invalid


// not necessary with bluebird
function only(type, handler) {
  return function(e) {
    if (e instanceof type) return handler(e);
    else throw e;
  }
}

function getQueryReport(q) {
  var timing = Date.now();
  return db.query(q).then(function(results) {
    return new ResultsReport(results, timing);
  }).catch(only(QueryError, function (e) {
    // this error was expected, make sure we show it to the user
    return new ErrorReport(e, timing); 
  }));
  // all other errors are automatically propagated
}

But we digress.

ForbesLindesay commented 10 years ago

I always write .then(null, handler) rather than .catch(handler) but that's just personal preference.

Node could use a nice graphical debugger with step through and live dev tools that was good enough for everyone to use it all the time. That still doesn't help much for surfacing errors in production systems though, which done does?

medikoo commented 10 years ago

On my side I'd rather make use of then second argument and handle errors (that are to be handled) directly after they occur. It's unlikely that same kind of error handling will repeat across the chain, and it's much more bulletproof approach:

return db.query(q).then(function(results) {
  // Let eventual report error propagate freely
  return new ResultsReport(results, timing);
}, ifErr(AcceptableQueryError, queryErrorHandler));

If additionally if I would like to handle some specific ResultsReport error, and it's sync operation I follow standard sync way to do that:

return db.query(q).then(function(results) {
  try {
    return new ResultsReport(results, timing);
  } catch (e) {
    // Report error
    return ifErr(AcceptableReportError, reportErrorHandler)(e);
  }
}, ifErr(AcceptableQueryError, queryErrorHandler));
juandopazo commented 10 years ago

One thing that done addresses very well is normalization to a callback API. Let´s say we have an API that uses promises internally. In order to expose the result of calling that API in terms of callbacks, we need to jump through some bureaucratic hoops:

function someCallbackAPI(foo, callback) {
  return doSomething(foo)
    .then(somethingElse)
    .then(moreWork)
    .then(function (result) {
      setImmediate(callback.bind(null, null, result));
    }, function (error) {
      setImmediate(callback.bind(null, error));
    });
}

function sameUsingDone(foo, callback) {
  return doSomething(foo)
    .then(somethingElse)
    .then(moreWork)
    .done(callback.bind(null, null), callback);
}

I don't think this use case is going away any time soon.

joliss commented 10 years ago

I don't know whether .done is the right solution. However, I'd like to point out something to everyone who's thinking about this stuff:

Handling exceptions in asynchronous code properly is not only a usability issue. Rather, I believe that it's important from a security point of view. Allow me to elaborate.

Common wisdom seems to be that when there is an uncaught exception in a Node server app, keeping the server running will cause issues and leak resources, so you should simply try to gracefully shut down the server process, and start a new one. This means that if an attacker can figure out a way to trigger an uncaught exception in your app, they might be able to force you do expend a significant amount of CPU time by sending a specially crafted request. As a result, any remotely triggerable uncaught exception creates a denial of service vulnerability, because an attacker can leverage a low amount of network bandwidth into a high amount of CPU usage. A past example of this CPU-to-network leverage issue would be the slew of vulnerabilities discovered in 2011, caused by dictionaries implemented without collision-resistant hash functions; see the original slides on this, especially pp. 50 ff.

In a Rails app, having an uncaught exception in your simply results in a 500 page. In a Node app, it would seem to cause a vulnerability under typical conditions. This is disastrous for security, because it turns ordinary bugs into vulnerabilities.

Even worse, note that libraries tend to accumulate "dead" attack surface. For instance, for the longest time Rails was parsing YAML, even though almost nobody was using it. YAML support was what I call "dead" attack surface, because clearly it wasn't used anymore. I presume that it was kept to avoid breaking compatibility -- perhaps reasonably so. Of course YAML parsing turned out to be ludicrously insecure in the end, and that's how we found out about it. But think about how many other such arcane code paths are exposed by library code in an average application. Most shouldn't contain egregious vulnerabilities of the YAML kind, but it would be very strange indeed if none had conditions under which they'd throw exceptions. Note also that it's very hard to guard against this by auditing.

To summarize, I suspect that the average Node server application contains several undiscovered DoS vulnerabilities owing to this problem. I'm not sure how to guard against this either.

My hope would be that in the future, we can use promises to evolve error handling in such a way that in most or all places, code will be able to safely throw unexpected exceptions without causing a vulnerability.

At the moment however, it seems to me that even with promises, there are still places where your code simply mustn't throw an error, and if it does, you are in a tricky spot, potentially leading to vulnerabilities as described above.

I don't have a solution, but I just wanted to point out with this comment that if we manage to solve the problem of uncaught exceptions with promises, it would not only be good for developer usability. Rather, it might also be an unexpected boon for the security of Node applications in the long run.

tl;dr: Node apps seem to suffer from a common class of DoS vulnerability. Promises don't avert this at the moment, but they might in the future, if we can figure out how.

Update: Now regarding the specific .done proposal, it seems to me that it's advocating exactly the problem I described above: Throwing unhandled exceptions into the event loop. So no, .done is definitely not the right solution.

Thanks to @stefanpenner for proof-reading a draft of this.

wycats commented 10 years ago

@joliss my thinking about what will end up being idiomatic is this:

spawn(function*() {
  let currentUser = yield User.find(currentUser);
  let post = yield Post.find(params.id, { user: currentUser });
  // more code leading up to generation of JSON, all of which uses yield
});

Because generators natively support proper synchronous exception handling, the class of problem that leads to swallowed errors will generally not occur when using promises together with generators. As a result, I think it would be best if we held off on making too many assumptions about how the error-swallowing behavior will affect idiomatic usage of promises, and stick with the simple one-API (.then).

TL;DR I think generators and yielding to promises will eliminate this class of problem, so we should hurry up that future and not lard up the current API with solutions to scenarios that will go away when generators become idiomatic.

joliss commented 10 years ago

@wycats's elaboration from IM, in case other people are confused about how the spawn + generator thing works:

wycats: The way spawn works is wycats: yield somePromise wycats: Will pause the generator until the promise is resolved or rejected wycats: If resolved, it yields the value wycats: If rejected, it throws in the current scope wycats: So there is no .then chaining wycats: Ever wycats: .then only happens internally in spawn

spion commented 10 years ago

@wycats unfortunately, synchronous catch in JavaScript doesn't support any kind of error filtering. Afaik, catch-all is an antipattern in all languages - one should only catch expected errors. Error bubbling makes this a real issue - at the topmost levels all kinds of errors may be aggregated and we definitely have no idea what they may all be.

Promise libraries like Bluebird alleviate this by allowing

.catch(ErrorType | predicateFn, handler);

But its also equally easy to write a combinator

let ifErr = (predicate, handler) => e => { if (predicate(e)) return handler(e); throw e; }

and use it with other libraries

.catch(ifErr(predicateFn, handler))

Since JS catch wont have pattern matching until ES7, we will have to resort to

let onlyErr = (filter, e) => { if (!filter(e)) throw e; }

and

try {
  yield fn1();
  yield fn2();
} catch (e) { onlyErr(predicate, e);
  only handle errors that satisfy the predicate
}

The difference is obvious - its much easier to evolve libraries. If other promise libraries find Bluebird's error filtering style useful, they can adopt it. Its equally easy for some of these libraries to even forbid catch without predicate (that might be a good idea for an experiment)

In contrast, it would take a huge amount of time (until ES7) for JS to incorporate error filtering, and there is unfortunately not much opportunity for experimentation there.

I like generators, but because of the above its likely that I'll wait for ES7 before reconsidering them again, and even then I'll probably prefer the flexibility of functional style - especially after the arrival of arrows in ES6

ForbesLindesay commented 10 years ago

It is normal in virtually all websites to handle virtually all un-caught exceptions by sending a 500 page to the user and not re-starting the app.

medikoo commented 10 years ago

Recently on a local meetup I made the presentation about asynchronous interfaces in JavaScript, it covered also introduction to promises with all surrounding controversies.

One of the ideas was to explain promises starting with done, make it a center point of implementation, and on that basis explain then. It felt right logically, and in turn produced simple and very comprehensible implementation, see medikoo/plain-promise

kriskowal commented 10 years ago

@medikoo I have done this refactoring in Q for the experimental branch https://github.com/kriskowal/q/blob/v2/q.js#L748-L874

cscott commented 10 years ago

The prfun library implements Promise#done on top of standard ES6 promises (using the es6-shim implementation if necessary). It is definitely useful in the short term, although I agree with the comments above that it doesn't need to be written into the ES6 spec since it's not clear that it's the correct long-term answer.

briancavalier commented 10 years ago

@medikoo @kriskowal I'll pile on. when.js 3.x does basically the same thing. It implements both then and done on top of a function that is much more like done than it is like then.

panuhorsmalahti commented 9 years ago

One problem with "log to console"/"log to live panel of unhandled promises" instead of .done is that if you're using .done you can catch that error and restart the Node.js process (or just let the process die as is the default). However, restarting a Node.js process from a single "unhandled promise logged to the console" is probably not the default. In addition, from the discussion above it seems that there will be false positives, and that probably leads to most people not enabling the "restart" functionality in production or otherwise.

The result of this, I fear, is that there will be production and development Node.js applications that contain errors that are not noticed opening security issues.

benjamingr commented 9 years ago

@panuhorsmalahti actually I performed research on that today for Bluebird (which offers both options and also lets you set what happens when an possiblyUnhandledRejection occurs) - most people (around 70%) logged an unhandled rejection (and didn't crash), a bunch (around 20%) did crash the process.

In any way the errors will not be "suppressed".

xtianus79 commented 8 years ago

Anymore debate on this .done() topic? Very interesting read. Once question I have which may be stupidly obviously but is the use of .done to replace or finish a .then() chain? .then().then().then().done(); <<< ???

Or can you chain them? Perhaps I am just thinking of this wrong but it is mentioned many times but not well understood I guess for me of ... "know where and when to use .done()?"

Just wanted to update:

I found this site here which explained it very well... and I get the point of what he( @ForbesLindesay ) is trying to say...

In other words, .done() is not using a promise which requires a value but rather a returning function based on it's own merits??? Is done semantically the correct phrase here? it implies a few things that I venture to be ambiguous.

  1. The promise chain/transaction is finished... is that true? Can you chain done()'s? Can you make a done(). followed by a then()? would you ever want to do that?
  2. It does not imply that you can't use the value/result of the preceding promise.

(( I would call them loaded promises and empty promises(haha) no... seriously unloaded promises. ))

In reality couldn't I then have a .thenL and .thenUl for my purposes? I get the .done this supposed to fire off an un-handled error into the event loop but in chaining purposes what if you want that in general. Again, am I supposed to chain .done()'s? that is very confusing and not very semantic.

if the pure purpose is to use an unnecessary resulted promise to then have an error come to the event loop this naming convention is odd. If it is to do this purely at the end of a promise chain which is then to use an unnecessary resulted promise that reports to the event loop --- well isn't that a catch?

One last point, with the .thenL() and .thenUl() framework the chaining becomes obvious but also a .thenDone() could finalize and finish the chain operation that then has any other special functionality besides being at the end and being done and being able to report an error to the event loop?

This could prevent promise nesting hell. Your thoughts?

reference: https://www.promisejs.org/

Put simply, .then is to .done as .map is to .forEach. To put that another way, use .then whenever you're going to do something with the result (even if that's just waiting for it to finish) and use .done whenever you aren't planning on doing anything with the result.

xtianus79 commented 8 years ago

Wow this kind of throws cold water on that whole thing no or this just in relation to bluebird?

https://github.com/petkaantonov/bluebird/issues/471

medikoo commented 8 years ago

@xtianus79 done simply is just plain way to access resolved value. So if you do not need a promise returned by then (in other words: you don't need to extend then chain), then it's cleaner to use done. It shouldn't be understood anything more than that.

This issue also promotes done as a solution for error swallowing, but I agree now, that introduction of done doesn't solve this issue well. Firstly there are cases where superfluous done() calls still will be required from a programmer (e.g. writeFile(path, content).done()), and secondly programmer shouldn't be punished for using then mistakenly instead of done (same as he/she is not punished when using map over forEach when there's no need for)

ForbesLindesay commented 8 years ago

To answer your main question: no, you cannot chain .dones.

You suggest that the normal behaviour you want is for errors to be re-thrown/reported. I dispute that claim somewhat. The normal behaviour you want is for all your errors to clime the stack of functions that called the code that threw an error. I don't want an error reported in readFile if I later handle it in readSettingsFile by simply returning the default settings. The time I do want error to be re-thown/reported is at the top of the chain. For example, someone clicks a "vote" button in my app:

voteButton.addEventListener('click', function () {
  request('post', '/api/vote').then(function () { // here I'm transforming the result of the first request, by making a second request
    return request('get', '/api/updated-data');
  }).done(function (data) { // here, I'm done adding handlers.  The last thing I'm doing is updating the UI.
    updateUI(data);
  });
});