Promise.prototype.done #19

Closed
medikoo opened this Issue Sep 4, 2013 · 149 comments

Comments

Projects
None yet
@medikoo

medikoo commented Sep 4, 2013

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:

  • Affects performance:
    • Creates promise objects we don't need
    • forces processing to further tick
  • Unintuitive
  • Verbose

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.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Sep 4, 2013

Collaborator

The outcome of the previous TC39 discussion was to get rid of done() for now. It might resurface one way or another, but adding it back before revisiting that discussion seems bad, and we can easily revisit that after we ship the basic design.

Collaborator

annevk commented Sep 4, 2013

The outcome of the previous TC39 discussion was to get rid of done() for now. It might resurface one way or another, but adding it back before revisiting that discussion seems bad, and we can easily revisit that after we ship the basic design.

@medikoo

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo Sep 4, 2013

@annevk Thanks for clarification. I just wanted to point that without that, spec (even in most basic form) shouldn't be perceived as complete.

medikoo commented Sep 4, 2013

@annevk Thanks for clarification. I just wanted to point that without that, spec (even in most basic form) shouldn't be perceived as complete.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Sep 4, 2013

Owner

done's job will be done by integrating unhandled rejection tracking functionality into dev tools. Most TC39ers, from what I understand, as well as myself, perceive that as enough for the spec to be complete.

Furthermore, there is no need for implementations to create promise objects only to have them immediately GCed. Unlike user-space promise implementations, that is easily detectable; such then invocations whose return values are ignored can simply not create a new promise object.

Owner

domenic commented Sep 4, 2013

done's job will be done by integrating unhandled rejection tracking functionality into dev tools. Most TC39ers, from what I understand, as well as myself, perceive that as enough for the spec to be complete.

Furthermore, there is no need for implementations to create promise objects only to have them immediately GCed. Unlike user-space promise implementations, that is easily detectable; such then invocations whose return values are ignored can simply not create a new promise object.

@domenic domenic closed this Sep 4, 2013

@medikoo

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo Sep 4, 2013

done's job will be done by integrating unhandled rejection tracking functionality into dev tools. Most TC39ers, from what I understand, as well as myself, perceive that as enough for the spec to be complete.

Monitoring solution significantly raises complexity of implementation, and brings many other issues. I haven't seen it yet either implemented or specified in a way that would really solve above issue.

It's really hard to understand for me, why you advocate after something that is difficult to implement, not logical and not natural for JavaScript, when real solution can be so simple, and as far as I remember from discussion on es-discuss, many were concerned about that.

Furthermore, there is no need for implementations to create promise objects only to have them immediately GCed. Unlike user-space promise implementations, that is easily detectable

Can you explain exactly, how? Static code analysis?

medikoo commented Sep 4, 2013

done's job will be done by integrating unhandled rejection tracking functionality into dev tools. Most TC39ers, from what I understand, as well as myself, perceive that as enough for the spec to be complete.

Monitoring solution significantly raises complexity of implementation, and brings many other issues. I haven't seen it yet either implemented or specified in a way that would really solve above issue.

It's really hard to understand for me, why you advocate after something that is difficult to implement, not logical and not natural for JavaScript, when real solution can be so simple, and as far as I remember from discussion on es-discuss, many were concerned about that.

Furthermore, there is no need for implementations to create promise objects only to have them immediately GCed. Unlike user-space promise implementations, that is easily detectable

Can you explain exactly, how? Static code analysis?

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Sep 4, 2013

Collaborator

Implementations can report unhandled exceptions during GC to the error console.

Collaborator

annevk commented Sep 4, 2013

Implementations can report unhandled exceptions during GC to the error console.

@medikoo

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo Sep 4, 2013

@annevk You mean that when promise would be GC'ed, implementation would check whether reject value was retrieved and if not, it would report to reject value to error console (?)

What if implementation would completely implement this spec, but won't provide this functionality?
I hope you understand that such implementation would turn out as hardly usable.

...and again it's not error handling that JS programmers are used to, it can be simple and should be in my opinion.

medikoo commented Sep 4, 2013

@annevk You mean that when promise would be GC'ed, implementation would check whether reject value was retrieved and if not, it would report to reject value to error console (?)

What if implementation would completely implement this spec, but won't provide this functionality?
I hope you understand that such implementation would turn out as hardly usable.

...and again it's not error handling that JS programmers are used to, it can be simple and should be in my opinion.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Sep 4, 2013

Collaborator

If implementations do not provide adequate debugging, they'll be used less. This is the same for any number of features on the platform. And simple is not necessarily good here, as people will use .then() and forget about things.

Collaborator

annevk commented Sep 4, 2013

If implementations do not provide adequate debugging, they'll be used less. This is the same for any number of features on the platform. And simple is not necessarily good here, as people will use .then() and forget about things.

@medikoo

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo Sep 4, 2013

@annevk only with done programmer can have full control over error handling, and promise libraries should provide that.

Argument that having both then and done is too much for programmer's head is quite controversial to me. Both methods serve two different use cases, and both are equally important.
Also looking at other API examples you can see it's not true. e.g. programmers somehow manage well both map and forEach on arrays. I haven't seen complains that we should remove forEach and just use map, as it's confusing and implementation might automatically not create returned arrays if it detects it's not taken.

medikoo commented Sep 4, 2013

@annevk only with done programmer can have full control over error handling, and promise libraries should provide that.

Argument that having both then and done is too much for programmer's head is quite controversial to me. Both methods serve two different use cases, and both are equally important.
Also looking at other API examples you can see it's not true. e.g. programmers somehow manage well both map and forEach on arrays. I haven't seen complains that we should remove forEach and just use map, as it's confusing and implementation might automatically not create returned arrays if it detects it's not taken.

@annevk

This comment has been minimized.

Show comment
Hide comment

@annevk annevk reopened this Sep 6, 2013

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Sep 6, 2013

Owner

Might be time for @wycats to step in, as he's one of the most prominent anti-done people.

Owner

domenic commented Sep 6, 2013

Might be time for @wycats to step in, as he's one of the most prominent anti-done people.

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Sep 6, 2013

I don't feel like arguing with BE. I strongly believe that this is a scenario solving mistake that we are hastily making as promises have gained popularity.

I have come to believe that the fact that done is needed at all is a fatal flaw in the current specification. Swallowing honest-to-god exceptions (TypeError, ReferenceError) as though they were promise rejections was just a mistake.

At minimum, it should be possible to configure a promise library to bubble out exceptions, for debugging.

I would like it if we didn't repeat this mistake with DOMPromises, and avoid compounding the mistake by complicating the API.

wycats commented Sep 6, 2013

I don't feel like arguing with BE. I strongly believe that this is a scenario solving mistake that we are hastily making as promises have gained popularity.

I have come to believe that the fact that done is needed at all is a fatal flaw in the current specification. Swallowing honest-to-god exceptions (TypeError, ReferenceError) as though they were promise rejections was just a mistake.

At minimum, it should be possible to configure a promise library to bubble out exceptions, for debugging.

I would like it if we didn't repeat this mistake with DOMPromises, and avoid compounding the mistake by complicating the API.

@mhofman

This comment has been minimized.

Show comment
Hide comment
@mhofman

mhofman Sep 6, 2013

I've been reading and thinking a lot about this error / rejection handling issue in recent days and I've become convinced that the answer is in keeping Promise's semantics simple so that it mirrors synchronous calls.

In that sense, then is used to execute another operation after the previous one completes and catch is used to handle an operation failing. For the same reason, I would also like to see finally as I expressed before in #18 but I guess I could live without it for now.

Discriminating on the failure's type is a dangerous business since I doubt we can be sure a TypeError / ReferenceError is always due to a promise's consumer. For example, what if such an error had actually bubbled from inside a library the consumer is using? Should the consumer's asynchronous flow be interrupted even if he had a catch in place for handling such errors?

I think the burden of making sure asynchronous operations are as easy to debug as synchronous ones lies with the browser / JavaScript engine. Logging unhandled rejections is the most obvious aspect of this but there are plenty of other things that can be done to improve the developer's experience. For example engines could provide the asynchronous call stack when using promises. This is a feature available in the newest Visual Studio 2013 and it supports WinJS promises (http://blogs.msdn.com/b/visualstudioalm/archive/2013/07/01/debugging-asynchronous-code-in-visual-studio-2013-call-stack-enhancements.aspx)

Asking the developer to finish their asynchronous flow with a done creates the potential for them to forget to do so just as much as they now forget to finish it with a catch.

mhofman commented Sep 6, 2013

I've been reading and thinking a lot about this error / rejection handling issue in recent days and I've become convinced that the answer is in keeping Promise's semantics simple so that it mirrors synchronous calls.

In that sense, then is used to execute another operation after the previous one completes and catch is used to handle an operation failing. For the same reason, I would also like to see finally as I expressed before in #18 but I guess I could live without it for now.

Discriminating on the failure's type is a dangerous business since I doubt we can be sure a TypeError / ReferenceError is always due to a promise's consumer. For example, what if such an error had actually bubbled from inside a library the consumer is using? Should the consumer's asynchronous flow be interrupted even if he had a catch in place for handling such errors?

I think the burden of making sure asynchronous operations are as easy to debug as synchronous ones lies with the browser / JavaScript engine. Logging unhandled rejections is the most obvious aspect of this but there are plenty of other things that can be done to improve the developer's experience. For example engines could provide the asynchronous call stack when using promises. This is a feature available in the newest Visual Studio 2013 and it supports WinJS promises (http://blogs.msdn.com/b/visualstudioalm/archive/2013/07/01/debugging-asynchronous-code-in-visual-studio-2013-call-stack-enhancements.aspx)

Asking the developer to finish their asynchronous flow with a done creates the potential for them to forget to do so just as much as they now forget to finish it with a catch.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Sep 6, 2013

Owner

I agree with everything @mhofman says, with the caveat that I am not sure what the exact solution would be for consoles (i.e. non-interactive debuggers). I would like a solution that does not affect the authoring-time experience, though.

Owner

domenic commented Sep 6, 2013

I agree with everything @mhofman says, with the caveat that I am not sure what the exact solution would be for consoles (i.e. non-interactive debuggers). I would like a solution that does not affect the authoring-time experience, though.

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Sep 6, 2013

The try/catch that wraps the success and failure handlers is catching errors that by definition were triggered by consumer code.

wycats commented Sep 6, 2013

The try/catch that wraps the success and failure handlers is catching errors that by definition were triggered by consumer code.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Sep 6, 2013

Collaborator

@erights, @BrendanEich, way in maybe? Still feels like something we can address later.

Collaborator

annevk commented Sep 6, 2013

@erights, @BrendanEich, way in maybe? Still feels like something we can address later.

@mhofman

This comment has been minimized.

Show comment
Hide comment
@mhofman

mhofman Sep 6, 2013

Yes, those errors are triggered by JavaScript code, but they don't automatically originate in the code written by the developer (consumer of the promise). My point was that a developer might want to handle such errors himself.

Regarding console type environments, couldn't the unhandled rejection be considered as a failure? I'm not sure of the consequences but for example right now an error in a setTimeout in node.js will cause the program to terminate.

mhofman commented Sep 6, 2013

Yes, those errors are triggered by JavaScript code, but they don't automatically originate in the code written by the developer (consumer of the promise). My point was that a developer might want to handle such errors himself.

Regarding console type environments, couldn't the unhandled rejection be considered as a failure? I'm not sure of the consequences but for example right now an error in a setTimeout in node.js will cause the program to terminate.

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Sep 6, 2013

@mhofman yep. Bottom line, many these issues are best handled by debugging environments, but as promises have quickly become more popular, and debuggers haven't caught up, people are trying to slam in the .done() solution to PATCH IT NOW.

I'm working on the Ember Promises debugger (for the Ember Inspector) and have talked with Dave Camp a bunch about plans for the Mozilla debugger, and making API decisions right now (before even the first iteration of debugger tools for promises have shipped) strikes me as very short-sighted.

That's why my proposals are almost all modes that you can turn on in debugging as a stopgap, and not permanent changes to the core spec.

wycats commented Sep 6, 2013

@mhofman yep. Bottom line, many these issues are best handled by debugging environments, but as promises have quickly become more popular, and debuggers haven't caught up, people are trying to slam in the .done() solution to PATCH IT NOW.

I'm working on the Ember Promises debugger (for the Ember Inspector) and have talked with Dave Camp a bunch about plans for the Mozilla debugger, and making API decisions right now (before even the first iteration of debugger tools for promises have shipped) strikes me as very short-sighted.

That's why my proposals are almost all modes that you can turn on in debugging as a stopgap, and not permanent changes to the core spec.

@Nathan-Wall

This comment has been minimized.

Show comment
Hide comment
@Nathan-Wall

Nathan-Wall Sep 7, 2013

As someone who spent a long time thinking throw -> reject was a mistake in promise design, I have done a 180 fairly recently and wholeheartedly support the no-done-patch sentiments in this thread. There was a time when browsers didn't have good debuggers for regular errors; that's just where we happen to be right now with promises. Adding done to the language just so that you can see certain errors in modern debuggers would be akin to teaching people to alert all errors in 2003 instead of using throw because they were practically invisible (or impossible to get the right message/line numbers from) back then. I mean, alert was what you had to use when you were debugging, but it would have been bad design to use it in general purpose code because it'd be impossible to recover from it. Since an error thrown in done is impossible to recover from, it would be bad design to use, and shouldn't be part of the language.

Like Anne said, implementations that don't provide good debuggers for promises won't be used to debug problems in async control flow (or they'll be patched with tools like Firebug).

As someone who spent a long time thinking throw -> reject was a mistake in promise design, I have done a 180 fairly recently and wholeheartedly support the no-done-patch sentiments in this thread. There was a time when browsers didn't have good debuggers for regular errors; that's just where we happen to be right now with promises. Adding done to the language just so that you can see certain errors in modern debuggers would be akin to teaching people to alert all errors in 2003 instead of using throw because they were practically invisible (or impossible to get the right message/line numbers from) back then. I mean, alert was what you had to use when you were debugging, but it would have been bad design to use it in general purpose code because it'd be impossible to recover from it. Since an error thrown in done is impossible to recover from, it would be bad design to use, and shouldn't be part of the language.

Like Anne said, implementations that don't provide good debuggers for promises won't be used to debug problems in async control flow (or they'll be patched with tools like Firebug).

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Sep 7, 2013

Contributor

I don't want .done for debugging purposes (that's a useful side effect). I want it for two reasons:

  1. It lets me be explicit about what I want to happen (just like I still use .forEach whenever I don't want to make use of the result I'd get via .map.
  2. It is an extremely useful tool for explaining .then. I just gave a talk (2 days ago) on promises and described .done first, and then described .then as a helper method for .done when you need a new promise as the result. Everyone in that room instantly understood how .then works and what it does. Explaining .then first doesn't work. Explaining .map without first explaining .forEach doesn't work.

We need .done so we can explain what .then does.

Contributor

ForbesLindesay commented Sep 7, 2013

I don't want .done for debugging purposes (that's a useful side effect). I want it for two reasons:

  1. It lets me be explicit about what I want to happen (just like I still use .forEach whenever I don't want to make use of the result I'd get via .map.
  2. It is an extremely useful tool for explaining .then. I just gave a talk (2 days ago) on promises and described .done first, and then described .then as a helper method for .done when you need a new promise as the result. Everyone in that room instantly understood how .then works and what it does. Explaining .then first doesn't work. Explaining .map without first explaining .forEach doesn't work.

We need .done so we can explain what .then does.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 8, 2013

@Nathan-Wall Your analogy to primordial JS debugging breaks down when you realize that the root problem is that under the current paradigm, there is no possible way to distinguish an "unhandled" rejection from a program error. "Monitoring" tools attempt to sidestep this difficulty by placing the burden of distinction upon the programmer at the time of debugging. But programming languages should be designed so that the programmer can state his or her intentions in the source code itself.

Some random thoughts:

  • One cannot ship an API that is not debuggable with built-in features.
  • One cannot ship an API that is debuggable only with features that have not been built yet, or have not been fully prototyped and tested.
  • There are significant problems with done, as stated by others in this issue.
  • Garbage collection of a rejected promise which has not been "handled" does not necessarily indicate a program error.
  • The best solution to this issue would likely require us to rethink our paradigm to some degree.

ghost commented Sep 8, 2013

@Nathan-Wall Your analogy to primordial JS debugging breaks down when you realize that the root problem is that under the current paradigm, there is no possible way to distinguish an "unhandled" rejection from a program error. "Monitoring" tools attempt to sidestep this difficulty by placing the burden of distinction upon the programmer at the time of debugging. But programming languages should be designed so that the programmer can state his or her intentions in the source code itself.

Some random thoughts:

  • One cannot ship an API that is not debuggable with built-in features.
  • One cannot ship an API that is debuggable only with features that have not been built yet, or have not been fully prototyped and tested.
  • There are significant problems with done, as stated by others in this issue.
  • Garbage collection of a rejected promise which has not been "handled" does not necessarily indicate a program error.
  • The best solution to this issue would likely require us to rethink our paradigm to some degree.
@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Sep 8, 2013

Collaborator

A lot of those seem false. We ship a lot of APIs that for debugging require non built-in features. E.g. lots of network APIs don't expose sufficient information for easy debugging (due to not wanting to reveal that information).

Collaborator

annevk commented Sep 8, 2013

A lot of those seem false. We ship a lot of APIs that for debugging require non built-in features. E.g. lots of network APIs don't expose sufficient information for easy debugging (due to not wanting to reveal that information).

@medikoo

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo Sep 8, 2013

Asking the developer to finish their asynchronous flow with a done creates the potential for them to forget to do so just as much as they now forget to finish it with a catch.

@mhofman It's not like that. The problem is that you treat done as a supplement of then which is logically wrong . then is a mapping function that developer should choose over done and not otherwise.
done should be treated as first choice function. It's the fundamental, straightforward way to access resolved value. It is then that someone may forgot about not done.

So, current state of the API provides us with brilliantly designed mapping function, but we miss one that allows us to process/access resolved values natural way, and agenda is to force user to use then for that, and fix its flaws with not natural, implementation dependent, mechanism that can't be implemented with plain JavaScript. Doesn't it give you notion there's some logical flaw here?

As a developer I expect that the API provides me with fine error handling control, not specifying done you're taking that away and I think there's no other JavaScript API that does that.

medikoo commented Sep 8, 2013

Asking the developer to finish their asynchronous flow with a done creates the potential for them to forget to do so just as much as they now forget to finish it with a catch.

@mhofman It's not like that. The problem is that you treat done as a supplement of then which is logically wrong . then is a mapping function that developer should choose over done and not otherwise.
done should be treated as first choice function. It's the fundamental, straightforward way to access resolved value. It is then that someone may forgot about not done.

So, current state of the API provides us with brilliantly designed mapping function, but we miss one that allows us to process/access resolved values natural way, and agenda is to force user to use then for that, and fix its flaws with not natural, implementation dependent, mechanism that can't be implemented with plain JavaScript. Doesn't it give you notion there's some logical flaw here?

As a developer I expect that the API provides me with fine error handling control, not specifying done you're taking that away and I think there's no other JavaScript API that does that.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Sep 8, 2013

Owner

@medikoo, I get that you have different ideas for how you would design and use a promise API. You fail to recognize that not everyone wants to use your library's paradigm.

Owner

domenic commented Sep 8, 2013

@medikoo, I get that you have different ideas for how you would design and use a promise API. You fail to recognize that not everyone wants to use your library's paradigm.

@medikoo

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo Sep 8, 2013

@domenic this has nothing to do with my library, it's implementation of done just follows others. It'll be also great if you'd have more open and constructive approach, your negative attitude doesn't help.

medikoo commented Sep 8, 2013

@domenic this has nothing to do with my library, it's implementation of done just follows others. It'll be also great if you'd have more open and constructive approach, your negative attitude doesn't help.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 9, 2013

@annevk True, to a certain extent. The difference, to me, is that when debugging this:

function someFunction() { ... }
promise.then(val => someFunctoin(val));

I need immediate feedback. I don't want to write boilerplate (i.e. done, or setTimeout) and I certainly don't want to tab over to a promise monitoring panel to figure out why nothing is happening. This is a program error. The programming environment should be able to figure out when I've made this kind of error and notify me accordingly.

I think punting on done is understandable, but it may be that by punting on done we assure it's victory. As @erights has said, a warty done still looks better that the proposed alternatives. And that has been the state of things for years. This indicates to me that something is missing from our current design with respect to error handling.

Is that missing something GC? I don't know, but I think it would be worth our time to explore the alternatives and implications.

ghost commented Sep 9, 2013

@annevk True, to a certain extent. The difference, to me, is that when debugging this:

function someFunction() { ... }
promise.then(val => someFunctoin(val));

I need immediate feedback. I don't want to write boilerplate (i.e. done, or setTimeout) and I certainly don't want to tab over to a promise monitoring panel to figure out why nothing is happening. This is a program error. The programming environment should be able to figure out when I've made this kind of error and notify me accordingly.

I think punting on done is understandable, but it may be that by punting on done we assure it's victory. As @erights has said, a warty done still looks better that the proposed alternatives. And that has been the state of things for years. This indicates to me that something is missing from our current design with respect to error handling.

Is that missing something GC? I don't know, but I think it would be worth our time to explore the alternatives and implications.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Sep 9, 2013

Collaborator

The error there is the typo in someFunction, right?

I don't understand how by punting on done we assure its victory. What strategy other than the implementation layer and something akin to done are there?

Collaborator

annevk commented Sep 9, 2013

The error there is the typo in someFunction, right?

I don't understand how by punting on done we assure its victory. What strategy other than the implementation layer and something akin to done are there?

@mhofman

This comment has been minimized.

Show comment
Hide comment
@mhofman

mhofman Sep 9, 2013

@zenparsing Unfortunately the behavior you're asking for is not possible with JavaScript. Consider a similar example without the use of promises:

function someFunction() { ... }
setTimeout(() => someFunctoin(123), 0);

The ReferenceError will only be thrown when the function passed to setTimeout is executed, not when setTimeout is called. Today, to figure out there is a typo here, you need the function to be executed and then look at your console. Pormise monitoring tools would offer the same kind of error reporting.

The only way to fail at parsing in this case would be to disallow functions dynamically added to the global from being executed.

mhofman commented Sep 9, 2013

@zenparsing Unfortunately the behavior you're asking for is not possible with JavaScript. Consider a similar example without the use of promises:

function someFunction() { ... }
setTimeout(() => someFunctoin(123), 0);

The ReferenceError will only be thrown when the function passed to setTimeout is executed, not when setTimeout is called. Today, to figure out there is a typo here, you need the function to be executed and then look at your console. Pormise monitoring tools would offer the same kind of error reporting.

The only way to fail at parsing in this case would be to disallow functions dynamically added to the global from being executed.

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Sep 9, 2013

Contributor
  1. forEach is the basic way of getting values out of an array, not map
  2. done should be the basic way of getting a value out of a promise, not then

I don't see how you can dispute the second statement without disputing the first. Does anyone dispute the first statement? Does anyone see a reason to dispute the second without disputing the first?

Contributor

ForbesLindesay commented Sep 9, 2013

  1. forEach is the basic way of getting values out of an array, not map
  2. done should be the basic way of getting a value out of a promise, not then

I don't see how you can dispute the second statement without disputing the first. Does anyone dispute the first statement? Does anyone see a reason to dispute the second without disputing the first?

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Sep 9, 2013

Contributor

That doesn't mean that map won't be more popular than forEach or that then won't be more popular than done, but that wouldn't be an argument not to have done, unless you also think we shouldn't have had forEach.

Contributor

ForbesLindesay commented Sep 9, 2013

That doesn't mean that map won't be more popular than forEach or that then won't be more popular than done, but that wouldn't be an argument not to have done, unless you also think we shouldn't have had forEach.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Sep 9, 2013

Owner

I don't think the promise <-> array analogy holds up very well under scrutiny.

Owner

domenic commented Sep 9, 2013

I don't think the promise <-> array analogy holds up very well under scrutiny.

@medikoo

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo Sep 9, 2013

@domenic can you be more constructive, do you have any valid arguments?

medikoo commented Sep 9, 2013

@domenic can you be more constructive, do you have any valid arguments?

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Sep 9, 2013

Collaborator

It's not about whether or not to return a new promise. It's about reporting errors. The forEach/map analogy has nothing on that.

Collaborator

annevk commented Sep 9, 2013

It's not about whether or not to return a new promise. It's about reporting errors. The forEach/map analogy has nothing on that.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 9, 2013

@mhofman I meant the same ReferenceError that setTimeout would report on the console. There's no reason why I should have to add boilerplate (done) or tab between two different views (the console and the promise monitor) in order to see that error. It is a programming error and should be presented to the programmer in the same way as other programming errors.

ghost commented Sep 9, 2013

@mhofman I meant the same ReferenceError that setTimeout would report on the console. There's no reason why I should have to add boilerplate (done) or tab between two different views (the console and the promise monitor) in order to see that error. It is a programming error and should be presented to the programmer in the same way as other programming errors.

@medikoo

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo Sep 9, 2013

@annevk
It's not only about that, thing that promise.then creates another promise that we totally don't need, affects for performance and is not clean. Also the reason of proposed error reporting is mapping feature of then, this comes out of it.

You want us to deal with all those side effects of map on each value access. I really wouldn't like to be forced to usesetTimeout with native API's to be able to handle errors normal way. It's bad idea to leave API in such state.

medikoo commented Sep 9, 2013

@annevk
It's not only about that, thing that promise.then creates another promise that we totally don't need, affects for performance and is not clean. Also the reason of proposed error reporting is mapping feature of then, this comes out of it.

You want us to deal with all those side effects of map on each value access. I really wouldn't like to be forced to usesetTimeout with native API's to be able to handle errors normal way. It's bad idea to leave API in such state.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Sep 9, 2013

Owner

promise.then will not create a new promise in any reasonable optimizing JIT, unless its return value is actually consumed.

Owner

domenic commented Sep 9, 2013

promise.then will not create a new promise in any reasonable optimizing JIT, unless its return value is actually consumed.

@medikoo

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo Sep 9, 2013

@domenic it's weird thing to provide imperfect solution and fix its issues that way. It's another mess to handle for compiler. Also mind that such thing can't be implemented in plain JavaScript (no polyfill possible).

medikoo commented Sep 9, 2013

@domenic it's weird thing to provide imperfect solution and fix its issues that way. It's another mess to handle for compiler. Also mind that such thing can't be implemented in plain JavaScript (no polyfill possible).

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 9, 2013

@annevk I'll try to explain...

First, let's forget about the browser and just consider the server. The appropriate thing for a console (Node) application to do when it encounters an unhandled error is to crash the program, reporting some error information. You don't fire up a monitor.

So the only way that the current promises spec will work in that environment is to include boilerplate (done), or to rely on the GC to deduce that an unhandled rejection will never be handled, and therefore constitutes a program error.

To my knowledge, we've not fully validated the GC approach. If it turns out that we can't rely on the GC, then the only other option for that environment is done.

The force which is pushing us toward the GC or done is the fact that, as currently specified, one may attach a listener to a rejected promise in an arbitrarily distant future. This is the property which is wreaking havoc with our ability to report simple programming errors. Is this property required?

ghost commented Sep 9, 2013

@annevk I'll try to explain...

First, let's forget about the browser and just consider the server. The appropriate thing for a console (Node) application to do when it encounters an unhandled error is to crash the program, reporting some error information. You don't fire up a monitor.

So the only way that the current promises spec will work in that environment is to include boilerplate (done), or to rely on the GC to deduce that an unhandled rejection will never be handled, and therefore constitutes a program error.

To my knowledge, we've not fully validated the GC approach. If it turns out that we can't rely on the GC, then the only other option for that environment is done.

The force which is pushing us toward the GC or done is the fact that, as currently specified, one may attach a listener to a rejected promise in an arbitrarily distant future. This is the property which is wreaking havoc with our ability to report simple programming errors. Is this property required?

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Sep 10, 2013

Collaborator

Let's make that more concrete.

The suggestion would be that in PropagateToDerived(p) if p.[[Derived]] is empty and promise.[[Reason]] is set, you'd propagate it out of the promise? (E.g. window.onerror, console, ...)

Collaborator

annevk commented Sep 10, 2013

Let's make that more concrete.

The suggestion would be that in PropagateToDerived(p) if p.[[Derived]] is empty and promise.[[Reason]] is set, you'd propagate it out of the promise? (E.g. window.onerror, console, ...)

@jakearchibald

This comment has been minimized.

Show comment
Hide comment
@jakearchibald

jakearchibald Jan 10, 2014

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.

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

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jan 10, 2014

Owner

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.

Owner

domenic commented Jan 10, 2014

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

This comment has been minimized.

Show comment
Hide comment
@spion

spion Jan 10, 2014

@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.

spion commented Jan 10, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo Jan 10, 2014

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

medikoo commented Jan 10, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Jan 10, 2014

@medikoo you are incorrect.

@medikoo you are incorrect.

@spion

This comment has been minimized.

Show comment
Hide comment
@spion

spion Jan 10, 2014

@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.

spion commented Jan 10, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo Jan 10, 2014

@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.

medikoo commented Jan 10, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@spion

spion Jan 10, 2014

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

spion commented Jan 10, 2014

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

@medikoo

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo Jan 10, 2014

@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.

medikoo commented Jan 10, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@spion

spion Jan 10, 2014

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.

spion commented Jan 10, 2014

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

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Jan 10, 2014

Contributor

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?

Contributor

ForbesLindesay commented Jan 10, 2014

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

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo Jan 10, 2014

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

medikoo commented Jan 10, 2014

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

This comment has been minimized.

Show comment
Hide comment
@juandopazo

juandopazo Jan 14, 2014

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.

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

This comment has been minimized.

Show comment
Hide comment
@joliss

joliss Jan 18, 2014

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.

joliss commented Jan 18, 2014

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

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Jan 18, 2014

@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.

wycats commented Jan 18, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@joliss

joliss Jan 18, 2014

@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

joliss commented Jan 18, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@spion

spion Jan 18, 2014

@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

spion commented Jan 18, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Jan 18, 2014

Contributor

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.

Contributor

ForbesLindesay commented Jan 18, 2014

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

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo Apr 9, 2014

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

medikoo commented Apr 9, 2014

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

This comment has been minimized.

Show comment
Hide comment
@kriskowal

kriskowal Apr 9, 2014

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

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

@cscott

This comment has been minimized.

Show comment
Hide comment
@cscott

cscott Apr 9, 2014

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.

cscott commented Apr 9, 2014

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

This comment has been minimized.

Show comment
Hide comment
@briancavalier

briancavalier Apr 9, 2014

@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.

@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

This comment has been minimized.

Show comment
Hide comment
@panuhorsmalahti

panuhorsmalahti Jan 7, 2015

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.

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

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Jan 7, 2015

@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".

@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

This comment has been minimized.

Show comment
Hide comment
@xtianus79

xtianus79 Apr 27, 2016

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 Apr 27, 2016

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

This comment has been minimized.

Show comment
Hide comment
@xtianus79

xtianus79 Apr 27, 2016

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

petkaantonov/bluebird#471

xtianus79 commented Apr 27, 2016

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

petkaantonov/bluebird#471

@medikoo

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo Apr 28, 2016

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

medikoo commented Apr 28, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay May 6, 2016

Contributor

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

ForbesLindesay commented May 6, 2016

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

@fatcerberus fatcerberus referenced this issue in svaarala/duktape Oct 17, 2016

Closed

Implement ES6 Promise support #1019

0 of 6 tasks complete

@zhouzhongyuan zhouzhongyuan referenced this issue in zhouzhongyuan/qa Feb 12, 2017

Closed

Yigo中相关概念解析 #9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment