Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support a "capping required" mode of operation; or allow reporting unheard rejections. #23

Closed
asutherland opened this issue Jul 30, 2011 · 8 comments
Assignees

Comments

@asutherland
Copy link

Although I try very hard to avoid letting success values or rejections fall on the floor, I keep screwing up, and it seems like Q could help make it more obvious when I am screwing up.

The defer() resolve() implementation knows when it is being resolved with no one around to hear because pending will be empty. I propose adding some kind of optional reporting in this case. This would allow my testing framework to both notice when:

  • an error occurs and no one is paying attention to the rejection when it pops out the top
  • a very dumb program logic bug has some code returning a promise but no one is actually hooked up

Obviously, there needs to be a way to actually "cap" the chain of promises. The current end() method does this in spirit, but I believe will still result in resolutions with an undefined value. (OTOH, undefined could just be made special in that it does not trigger the reporting logic.)

I recognize that there are two use-cases that would not be happy about this happening all the time:

  1. People more clever than me who don't want to have to cap everything because they don't screw up.
  2. Programming idioms where the "when" request registration with the promise does not happen nearly immediately and instead the promise is held onto for multiple ticks and registered with later. This may happen more frequently than I expect, even in Q-provided helpers.

The solution to both those problems is that this would be opt-in. Either you could provide a custom handler, or invoke "$Q.forBabies()" which would just throw, thereby relying on node's/the browser's built-in stuff.

@kriskowal
Copy link
Owner

This has been on the back of my mind for a while. Tracking resolved/unresolved promises is also good for what you did earlier for waterfall analysis. We definitely need something, perhaps the ability to create Q sandbox instances that can create a report of the outstanding promise graph.

var {Q, pool} = Q.create({debug: false});
var promise = Q.defer('annotation').promise;
assert.deepEqual(
   pool,
   [{promise: promise, annotation: 'annotation'}],
   'promise pool contents'
);
promise.resolve();
assert.equal(0, pool.length);

I'll be away for a week, so I will probably not be able to have a lively discussion for a while. That said, I do want to have more dialog with friends and supporters. We need to be a more tightly knit group and I need to be more conscious of how Q is being used in the wild.

@asutherland
Copy link
Author

Sandboxes and support for annotations would be excellent, although I should be clear that my biggest concern in this issue is that use of Q can lead to quietly eaten exceptions. The Mozilla platform (at least when dealing with the non-browser XPCOM stuff) loves to eat exceptions, and I don't want to go back to that, so it would be great to make sure that rejections (that are exceptions, at least) are always re-thrown if not explicitly handled by the user.

In any event, Q is working fantastically for me, especially with a minor hack patch to re-throw errors. Thanks again for all the great work on Q, have a good week away/off, and let me know if there's a mailing list you would like to use to discuss such issues.

(The XPCOM issue in a nutshell, if you are interested, is a 1) fundamental impedance mismatch between C++ and (transparently-exposed) JS and 2) the use of integer success/error codes in the C++ code. The XPConnect code allows JS code to obey the C++-style IDL interfaces, but has a hard call to make when an exception propagates upward, because it is able to translate the exception to an integer failure code, which may or may not be desired... it can't tell and can only either report every exception it sees or just translate them to integers. Unfortunately, that sucks quite extensively for developers. Q is in the much more advantageous position of being able to tell that there is no one explicitly handling/translating an error, and thus that it can/should be reported.)

@ghost ghost assigned kriskowal Aug 9, 2011
@johnjbarton
Copy link

I believe issue 30, #30, is a example of the problem being discussed here.

@jrburke
Copy link
Collaborator

jrburke commented Jun 7, 2012

This came up in volo too, and recently saw a link to domains in node which sounds similar to the pool approach that @kriskowal mentioned. I do not have anything helpful to add, just mentioning data points.

@domenic
Copy link
Collaborator

domenic commented Jun 7, 2012

WinJS has a similar solution to Q, where .done(f, r) is equivalent to Q's .then(f, r).end():

The latter has a particularly poignant quote:

In practical terms, this means that if you end a chain of promises with a then and not done, all exceptions in that chain will get swallowed and you’ll never know there was a problem! This can place an app in an indeterminate state and cause much larger problems later on. So, unless you’re going to pass the last promise in a chain to another piece of code that will itself call done, always use done at the end of a chain even for a single async operation.

In practice, we've adapted to the status quo, and adopt the simple rule to always either (a) return the promise; or (b) cap with .end(). We enforce this even for promises that have error handlers, just in case the error handler itself throws, and also to make the rule simple and easy to follow without thinking.

@kriskowal
Copy link
Owner

end() is very much a stopgap, which is why I favor it over .done(). There are two cases where we will find ourselves editing the end of promise chains. The first is when you have to add another step. After a done, you have to walk back and forth to switch the previous done to then and add a done. With end, you just chain a new then between the previous and the end. The latter case, when good visualization tools for the state of a promise graph are available, it is my hope that .end() calls will become vestigial and trivial to remove.

However, I still think there would be value in introducing promise domains (pools as it were, but I think domains is going to catch on).

@domenic
Copy link
Collaborator

domenic commented Dec 11, 2012

@kriskowal
Copy link
Owner

It’s time to put this issue to bed. There are three directions being pursued.

  1. Domains exist. This mostly obviates the need for pools, which would probably not be ergonomic in the long run.
  2. Discussion about end vs done has run its course.
  3. Options are being explored for debugger extensions that surface transient unhandled rejected and pending promises, either through a separate window or logging and unlogging.

Thanks, @asutherland. This is the longest lasting, most relevant issue we have.

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

No branches or pull requests

5 participants