Throw in promise-fulfillment swallows exception #30

johnjbarton · 2011-11-20T19:05:21Z

<html>
<head>
<script src="../../lib/q/q.js"></script>
<script>

// Accept a promise for load, and report the success or failure of the load
function foo(loaded) {
 return Q.when(
   loaded, 
   function glory(loaded){
     // Ooops the processing of success path fails!
     throw new Error("You lose");
     console.log("We have ", loaded);
   }, 
   function fail(err) {
     console.error("We have "+err);
   }
 );
};

// Create a promise for a load
function promiseLoaded() {
  var defer = Q.defer();
  window.addEventListener(
    'load', 
    function(event) {
      defer.resolve("success");
    }, 
    false
  );
  return defer.promise;
}

function main() {
  // foo returns a promise, but we don't look at the return so we lose.
  foo(promiseLoaded());
}

main();
</script>
</head>
<body>
<h1>Bad developer ergonomics: throw in promise fulfillment swallows exception</h1>
<p>May be related to <a href="https://github.com/kriskowal/q/issues/23">Issue 23 on github q</a>
</body>
</html>

johnjbarton · 2011-11-20T19:08:12Z

Arggh. I guess github issues are not plain text.
https://github.com/johnjbarton/Purple/blob/master/tests/promises/throwInFulfilled.html

kriskowal · 2011-11-21T00:02:37Z

Agreed. However, swallowing exceptions is desirable behavior since it unifies synchronous and asynchronous exception handlers. I wavered on this for a while, but Irakli Gozashvili convinced me it was necessary for the flipside developer ergonomics.

The present stopgap is to terminate all promise chains with a .end() call.

function main() {
  // foo returns a promise, but we don't look at the return so we lose.
  return foo(promiseLoaded());
}

main().end();

I am playing with the idea of having another stop-gap solution: using console.log to write a live array to the console. I would then add exceptions to the array when they are swallowed, and remove them when they are handled.

It would be much more awesome to extend Firebug with something like Causeway, where I could use something like a console.events API to expose the causal graph of promises: a bar for each promise from when it was created to when it was resolved, whether it was fulfilled or rejected, what other promises were waiting for it, the stack trace at the point of creation, the stack trace at the point of failure, that kind of thing.

(Issues are in markdown format. You can use Github’s proprietary extension to Markdown for literal bodies: three backticks on top and on bottom. If you want syntax highlighting, put the file extension on the same line as the initial backticks.)

johnjbarton · 2011-11-21T02:02:05Z

How about a Q.debug = true mode? The problem with .end() is of course you fail to call it just in the cases you need it.

I suppose to be realistic the ergonomics of Q are pretty horrible in that the call stack goes from "oh, my code" to "OMG what is this code?" A big step could be showing the dev the chain you describe. What is Causeway? (This is one of the areas I want to work on, tho not in Firebug. Same issues face all other async libs like require.js.)

kriskowal · 2011-11-21T21:58:20Z

Causeway is a distributed debugger.

http://wiki.erights.org/wiki/Causeway

MarkM talked about it in Belgium recently. http://www.youtube.com/watch?v=w9hHHvhZ_HY

http://www.erights.org/elang/tools/causeway/

johnjbarton · 2011-11-21T22:21:16Z

Thanks! Since we are in the same process I think we can do something non-post morteum

kriskowal · 2011-11-21T22:29:50Z

@johnjbarton I certainly have ambitions for a live visualization.

IgorMinar · 2011-11-29T06:26:01Z

The way I handle this issue in my Q-like mini implementation of deferreds/promises is by logging any exception thrown from success/error callbacks in addition to rejecting the derived promises with the exception being the rejection reason. So essentially, my stance is that an exception unhandled in a callback represents a programming error and should be logged. To signal that derived promises should be intentionally rejected, one must return a rejected promise instead of a value or an exception.

So Q could be fixed to differentiate between these two scenarios (currently it does not):

promise.then(function(val) { throw 'something went unexpectedly wrong' }).then(..); // log exception and reject derived promise
promise.then(function(val) { return Q.reject('I am unable to compute the value')}).then(..); // don't log anything, just reject derived promise

This approach has been working quite well for me so far and I believe that it is in the "spirit" of deferreds/promises.

From a quick glance at the code, a good place to start would be to log these exceptions:

q/q.js

Line 506 in 8ff517e

} catch (exception) {

q/q.js

Line 514 in 8ff517e

} catch (exception) {

Keep in mind that this is a breaking change, because throwing an exception and returning a rejected promise is not the same thing any more. So developers would likely have to go back and fix the old code that merely relies on throwing, rethrowing or unhandling an exception.

kriskowal · 2011-11-29T20:21:42Z

@IgorMinar this is how Q worked about a year ago. Unfortunately it’s harder to go back than to go forward.

IgorMinar · 2011-11-29T20:30:55Z

@kriskowal can share more details about why this logging was removed?

kriskowal · 2011-11-29T20:35:14Z

@IgorMinar I was logging the exception and forwarding a rejection. This led to dirty and confusing double-logging if the rejection was handled properly.

I was also of the mind that exceptions would be decreed to be programmer errors and rejections were for intentional errors. However, this did not pan out in practice. A lot of existing systems throw exceptions for non-error cases, and these do need to get trapped in rejections. Having all thrown exceptions reported in every frame is not an option, and terminating a services because of an asynchronously handled exception is not an option. It’s really quite tricky. It would be a much better option to just visualize the promise graph for debugging purposes.

IgorMinar · 2011-11-29T20:52:34Z

@kriskowal if an existing system communicates a result of an action by throwing an exception then the success callback should expect that (since its part of the contract of the api that it relies on). Isn't it reasonable to expect the developer of the success callback to wrap the call into try/catch and reject return rejection if an expected exception is thrown?

I actually use an alternative method - I made the logger configurable, so at runtime I can choose to display the exception logs during development and in production I silence them.

kriskowal · 2011-11-29T20:56:53Z

@IgorMinar It really just isn’t practical. A lot of these functions only throw in exceptional conditions, so a cautious developer would have to try/catch every third party function.

I really am considering a gamut of solutions. One of them discussed earlier with @asutherland would be to allow the promise library to be instantiated with logging emitters. It’s still on the table if someone has the time.

asutherland · 2011-11-30T00:23:09Z

Another useful causeway link (that I don't believe the above links reference) is a more recent JS version with live-ish examples:
http://code.google.com/p/causeway/

kriskowal · 2014-04-07T17:41:17Z

I believe this issue has played out. We have done() as an ugly stopgap for explicitly surfacing programmatic errors. We will have promise debuggers soon, for the other cases.

axelson · 2015-03-08T09:45:54Z

@kriskowal is there a write-up on how to use done() to avoid "exception swallowing"?

kriskowal · 2015-03-08T17:00:00Z

@axelson https://github.com/kriskowal/q#the-end

randallb · 2015-03-27T19:55:45Z

So I love Q, and prefer it, except this error handling case is really difficult on me.

https://github.com/tildeio/rsvp.js/#error-handling

In RSVP, they have an "error" event that fires, meaning as a user you can have some console.assert thing show any unhandled errors. I'd love to have some way of surfacing all exceptions at least while developing... would this be something you'd be interested in having in Q? Even if as "Q.onError` if there's no native event bus. (I haven't yet looked at code.)

kriskowal · 2015-03-27T19:59:27Z

@randallb Q.onerror is a thing that exists.

randallb · 2015-03-27T20:01:45Z

well... i fail at reading docs then. Thank you very much.

randallb · 2015-03-27T20:08:37Z

Ah it'll only catch errors if you use the .done() method. I was proposing trying to trigger some event handler whenever an error is caught in a promise chain.

kriskowal · 2015-03-27T20:11:57Z

@randallb Sorry, that went out too soon. It probably does not do what you want. Q.onerror gets called for unhandled rejections that would get rethrown. It does not report errors that have been caught but have not been handled. There is a limbo for errors, where they might be observed and handled in a future turn. In the trivial case:

var rejected = Q.reject(new Error('boo'));
Q.delay(1000, function () {
    return rejected;
}).done();

This is too trivial a case. The problem will show up if you use promise queues or asynchronous iterators / readers, since the producer and the consumer operate independently.

var reader = Reader([1, 2, 3, 4])
.map(function (value) {
    if (value % 2) throw new Error('how odd');
});

// later...
reader.next().catch(function (error) {
});

So while we could surface unhandled errors in the same event loop turn like RSVP, it would be noisy for legitimate cases.

I need to refresh my context, but there are ongoing plans to surface other events like, ondefer, onresolve, onreject, onhandle that would be able to drive an inspector that would show "pending deferred" promises and "rejected unhandled" promises for any given moment.

guiprav · 2015-04-21T22:58:19Z

If Q

held weak references to all promises upon creation,
and strong references to all errors upon rejection,
and dropped those error references whenever they're successfuly handled by a catch() clause,

couldn't we implement a function that runs once per event loop and throws unhandled errors if their corresponding promise has been garbage collected?

Weak references to promises that have been garbage collected but have no outstanding unhandled errors are just dropped as well.

Since this comes at a performance cost and not every JavaScript engine Q runs on would necessarily support ES6's WeakMaps or whatever we use to implement this, this feature could just be disabled (either explicitly by client code or implicitly, if Q detects weak references aren't supported), and people who either don't want it or can't use it can just keep using Q.done.

Couldn't that work pretty well?

Granted, if the object takes too long to be garbage collected, the program may keep running for a while before the exception is logged and/or the process is killed, but that's better than nothing in my case, at least.

domenic · 2015-04-21T23:03:03Z

Yes, that would work wonderfully. Unfortunately weak maps are not weak references.

guiprav · 2015-04-21T23:07:05Z

From https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/WeakMap:

Because of references being weak, WeakMap keys are not enumerable
(i.e. there is no method giving you a list of the keys). If they were, the list
would depend on the state of garbage collection, introducing non-determinism.
If you want to have a list of keys, you should maintain it yourself.

Are you talking about this? It looks like you can kind of do it if you maintain a list of things that should be on the WeakMap, and check them periodically, no?

domenic · 2015-04-21T23:08:06Z

How would you maintain that list? (Answer: using a strong reference.)

guiprav · 2015-04-21T23:10:50Z

True. How about adding unique IDs to promises and keeping a list of those, though?

domenic · 2015-04-21T23:11:54Z

How would you then find out if the promise is in the weak map?

guiprav · 2015-04-21T23:12:17Z

Crap, true. They're not enumerable. That sucks :(

kriskowal · 2015-04-21T23:29:47Z

@n2liquid WeakMap is what it is for good reasons and is deliberately not for this use-case. There is a proposed WeakRef that provides a post mortem notification callback. We had to wait for TC39 to specify an event loop to get Promise. We’ll have to wait for TC39 to specify garbage collection before we’ll get WeakRef. Maybe we’ll get it someday, but that sounds hard. Members TC39 would prefer not to introduce a feature that would increase the observable differences between engines and specifying GC could seriously paint the language into a corner.

guiprav · 2015-04-21T23:36:12Z

@kriskowal I see, it's a very tricky thing, indeed. But boy, I just don't feel safe that even things like TypeErrors would just be swallowed if I forget to use Q.done or yield the promise into a Q.spawn generator.

I guess dumping unhandled errors to the console before exiting the program is as good as it'll get for me (I'm not writing a server, so this is reasonable in this particular case).

constfun · 2015-06-16T19:02:22Z

// In browsers, uncaught exceptions are not fatal.
// Re-throw them asynchronously to avoid slow-downs.

https://github.com/kriskowal/q/blob/v1/q.js#L158

I'm sorry, but really?? This is all kinds of wrong. Not only are we swallowing exceptions, we're destroying the stack too. I don't know what the solution is, but this ain't it.

guiprav · 2015-06-16T19:10:21Z

I'm sorry, but really?? This is all kinds of wrong. Not only are we swallowing exceptions, we're destroying the stack too. I don't know what the solution is, but this ain't it.

I don't think rethrowing destroys the stack. The stack will be that of when the Error object was created.

constfun · 2015-06-16T19:30:47Z

e.stack is available (only sometimes, it seems) but as a string, which means I can't step up from it in the debugger and inspect values, etc.

ghost assigned kriskowal Nov 21, 2011

silverbucket mentioned this issue Oct 22, 2013

Unable to catch thrown errors from try/catch block, around a function that uses promising. nilclass/promising#4

Open

kriskowal closed this as completed Apr 7, 2014

ouhouhsami mentioned this issue Jun 4, 2014

Promise-fulfillment swallows exception wavesjs/waves-loaders#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw in promise-fulfillment swallows exception #30

Throw in promise-fulfillment swallows exception #30

johnjbarton commented Nov 20, 2011

johnjbarton commented Nov 20, 2011

kriskowal commented Nov 21, 2011

johnjbarton commented Nov 21, 2011

kriskowal commented Nov 21, 2011

johnjbarton commented Nov 21, 2011

kriskowal commented Nov 21, 2011

IgorMinar commented Nov 29, 2011

kriskowal commented Nov 29, 2011

IgorMinar commented Nov 29, 2011

kriskowal commented Nov 29, 2011

IgorMinar commented Nov 29, 2011

kriskowal commented Nov 29, 2011

asutherland commented Nov 30, 2011

kriskowal commented Apr 7, 2014

axelson commented Mar 8, 2015

kriskowal commented Mar 8, 2015

randallb commented Mar 27, 2015

kriskowal commented Mar 27, 2015

randallb commented Mar 27, 2015

randallb commented Mar 27, 2015

kriskowal commented Mar 27, 2015

guiprav commented Apr 21, 2015

domenic commented Apr 21, 2015

guiprav commented Apr 21, 2015

domenic commented Apr 21, 2015

guiprav commented Apr 21, 2015

domenic commented Apr 21, 2015

guiprav commented Apr 21, 2015

kriskowal commented Apr 21, 2015

guiprav commented Apr 21, 2015

constfun commented Jun 16, 2015

guiprav commented Jun 16, 2015

constfun commented Jun 16, 2015

Throw in promise-fulfillment swallows exception #30

Throw in promise-fulfillment swallows exception #30

Comments

johnjbarton commented Nov 20, 2011

johnjbarton commented Nov 20, 2011

kriskowal commented Nov 21, 2011

johnjbarton commented Nov 21, 2011

kriskowal commented Nov 21, 2011

johnjbarton commented Nov 21, 2011

kriskowal commented Nov 21, 2011

IgorMinar commented Nov 29, 2011

kriskowal commented Nov 29, 2011

IgorMinar commented Nov 29, 2011

kriskowal commented Nov 29, 2011

IgorMinar commented Nov 29, 2011

kriskowal commented Nov 29, 2011

asutherland commented Nov 30, 2011

kriskowal commented Apr 7, 2014

axelson commented Mar 8, 2015

kriskowal commented Mar 8, 2015

randallb commented Mar 27, 2015

kriskowal commented Mar 27, 2015

randallb commented Mar 27, 2015

randallb commented Mar 27, 2015

kriskowal commented Mar 27, 2015

guiprav commented Apr 21, 2015

domenic commented Apr 21, 2015

guiprav commented Apr 21, 2015

domenic commented Apr 21, 2015

guiprav commented Apr 21, 2015

domenic commented Apr 21, 2015

guiprav commented Apr 21, 2015

kriskowal commented Apr 21, 2015

guiprav commented Apr 21, 2015

constfun commented Jun 16, 2015

guiprav commented Jun 16, 2015

constfun commented Jun 16, 2015