kriskowal / q

A promise library for JavaScript
MIT License
14.94k stars 1.2k forks source link

onFulfilled callbacks are executed after onRejected. #240

Closed Redsandro closed 11 years ago

Redsandro commented 11 years ago

The Promise Spec mentions about rejection:

If/when promise is rejected, respective onRejected callbacks must execute in the order of their originating calls to then.

However, after the first onRejected callback, onFulfilled callbacks are also called. Is this supposed to happen?

#!/usr/bin/env node

var util    = require('util'),
    q       = require('q');

q.fcall(function () {
    console.log('Throwing error');
    throw new Error('Catch!');
    return true;
})
.then(function () {
    console.log('You shall skip this step!');
    return true;
})
.fail(function (err) {
    console.log('I caught ya! :D');
})
.then(function () {
    console.log('Shouldn\'t this step be skipped?');
})
.done();
Throwing error
I caught ya! :D
Shouldn't this step be skipped?
domenic commented 11 years ago

.fail(onRejected) returns a new promise (it's shorthand from .then(undefined, onRejected)). The relevant section of the spec is thus 3.2.6, governing the behavior of the returned promise.

That new promise is fulfilled, since you implicitly returned undefined from the onRejected handler. If you had thrown an exception, the new promise would be rejected, and that step would be skipped, but since you returned, you're signaling that you handled the error.

Here is the analogous synchronous code:

try {
  console.log('Throwing error');
  throw new Error('Catch!');

  console.log('You shall skip this step!');
} catch (e) {
  console.log('I caught ya! :D');
}

console.log('Shouldn't this step be skipped?');
Redsandro commented 11 years ago

Thank you. I misunderstood again. The example clarifies the facts.

(Skip below, you replied at the same time.)

Original message:

I am trying to have different error handling for thrown errors further down the chain, but the onFulfilled is getting in my way. The spec doesn't explicitly mention that onFulfilled should execute after the first onRejected. The promise and me are in disagreement about whether or not this means the promise is allowed to trespass on fulfilled territory.

Redsandro commented 11 years ago

(edited above message)

Ah, I see how to get it to work how I assumed it did. Errors don't bubble, you have to rethrow them. (Just mentioning this for google, duckduckgo and other referencing purposes.)

.fail(function (err) {
    console.log('I caught ya! :D');
    throw err;
})
.then(function () {
    console.log('Shouldn\'t this step be skipped?');
})
.fail(etcetera)
.done();
domenic commented 11 years ago

Errors bubble! But if you handle them, then they're handled! :)

Redsandro commented 11 years ago

Yeah, I mean like.. handled errors don't bubble between onRejected handlers like oldschool web2.0 handled mouseclicks still bubble; ignoring the onFulfilled handlers for a moment as if they are like axons covered in myelin. Or something. You are right.

Redsandro commented 11 years ago

Is there a way to cancel the bubble, end the chain, without breaking it in two?

Because even if an error is handled and the chain goes on, the promised data/request/file/anything will not be available, and all the consecutive ticks are more often than not futile without this data. Only cause more errors to handle.

// ...
.fail(function (err) {
    console.log('Some exception that should end the chain.');
})
.then(function () {
    console.log('Chain should only go on if the above exception did not occur.');
})
.then() // etc

Like a conditional done()

Q.fcall(step1)
.then(step2)
.fail(okNeverMind).done() // Never mind the rest of the chain, but only when failed
.then(step3)
.then(step4)
.then(stepFinal, handleStep3and4)
.done();

My current alternative looks like this:

/// stuff ...
.fail(function (err) { // First handler
    console.log('httpRequest without Data was just a probe for JSON headers.');
    throw new Error('ignore');
})
.then() // blah
.done(
    function() {
        // We are done.
    }, function(err) {
        if (err.message == 'ignore') return;
        // Else handle real error
    }
)
domenic commented 11 years ago

I'd suggest something like

// ...
.fail(function (err) {
    console.log('This is not a real error. etc.');
    return false;
})
.then(function (value) {
    if (value !== false) {
        // do normal stuff
    }
})

Assuming that ... will fulfill with a non-false value in non-error conditions.

As always, it's good to think how you would handle this in the sync case. If you end up in a catch block for something that's not a real error, but this would affect whether or not subsequent steps are executed, you'd probably set a flag and use that to modify the subsequent steps---like we're doing here.

Redsandro commented 11 years ago

I have been editing my post without realizing you had replied already, but you got the gist.

The problem with your idea, I've been playing with that too, is that promises are a form of syntactic sugar. I am a big fan of syntactic sugar.

Imagine a longer chain:

fcall()
.fail() // Handle initial errors and stop the chain
.then()
.then()
.then()
.fail() // Handle errors in the above three steps and stop the chain
.then()
.then()
.then()
.done() // Handle errors in the final steps

Adding the probe to all these functions kinda introduces syntactic "salt" and 'complicates' use of existing functions that also have use in non-promise routines.

Silently bubbling the error so that it skips fulfilled-handlers seems like the better idea at the time, but it has me wondering if there should be a command like breakPromise or something that just stops the entire chain.

It's not spec, but also not crazy. It would be useful. Enhancement maybe.

domenic commented 11 years ago

I think we'd be best served if you could come up with an example of what you're trying to do, but synchronous. Remembering that promises are just an asynchronous parallel to synchronous function calls, that should help.

Redsandro commented 11 years ago

Well my current code is kind of huge so I tried to illustrate it with pseudo-code, but I am trying to simplify the initial chain for sugar reasons. First only for "inversing the inversion of control" (sync style for async code), but going a bit further now, it made me think: Either it's already possible or it's a great enhancement if it would be possible to break. It is compatible with the promise spec after all.

For example, one of the first steps is a promise for JSON data. But the node server.request fires an empty onData and onEnd event in certain cases because the client wants to check for a JSON accept header, for example when the client uses jQuery. The chain is nearly perfect, except for handling this (non)error or continuing otherwise. Further along the chain I have something similar when the promise reached a state where it's complemented with database data. In some rare cases it should stop because data is already written to server.response.

domenic commented 11 years ago

I look forward to seeing your synchronous counterpart code so I can help you further.

Redsandro commented 11 years ago

I think the answer to "can you break" is "no", and all I can do is suggest it would be a valuable function to add. We both provided workarounds, so I am not sure if dumping more code will be useful. I feel that people don't usually appreciate dumping of code that is more than a few lines.

But if you think you might get additional ideas, I'll gladly show you the request I am working with now, right after it comes from the router.

By now people usually comment on my object notation style as not how it is supposed to be, but hey I'm a rebel.

The listening server is routed to onIncomingRequest.

module.exports = {

    onIncomingRequest :     function(httpRequest, httpResponse) {

        // Pass around one single object
        var requestObj = {
            req: httpRequest,
            res: httpResponse
        };

        // Here is the chain. Start by reading the request
        self.readJsonRequest(httpRequest, httpResponse).then(
            function(data) {
                // Store JSON in requestObj
                requestObj.data = data;
                return requestObj;
            },
            function(err) {
                var msg = 'Ignoring request: '+ err.message;
                util.debug(msg);
                throw new Error('ignore');
            }
        )
        .then(db.promises.request.handleRequest)
        .then(db.promises.request.storeRequest)
        .fail(db.promises.request.handleMissing) // Rethrow error
        .then(db.promises.response.storeRequest)
        .done(
            function() {
                // We are done.
                //TODO: Do we need to do some sort of garbage collection?
            },
            function(err) {
                if (err.message == 'ignore') return;

                util.debug('error', 'Lookuop failed.', err.message); //TODO: Proper handler
            }
        );
    },

    // Private functions

    readJsonRequest     : function(request, response) {

        var data = '';
        var promise = q.defer(); I am suggesting 

        request.on('data', function(chunk) {
            data += chunk;
        });

        request.on('end', function() {

            if (data) {
                try {
                    data = JSON.parse(data);
                } catch (e) {
                    promise.reject(new Error('Not JSON'));
                }

                promise.resolve(data);
            }
            else {
                promise.reject(new Error('Empty request is a JSON handshake. Sending headers.'));
                response.writeHead(200, {'Content-Type': 'application/json', 'Access-Control-Allow-Headers': 'Content-Type'});
                response.end();
            }
        });
        return promise.promise;
    }
};
extend(exports, module.exports);

(Yes storeRequest is called twice, does a switch and an upsert, not important.)

Redsandro commented 11 years ago

Kinda boring right? Nevermind, I'll stick to 'ignore' error messages for now.

domenic commented 11 years ago

You never showed me synchronous code that emulates your desired workflow, which I asked for a few times under the reasoning that once I have a synchronous version, I can show you the optimal promise-based asynchronous counterpart. If you show me synchronous code that does what you want (e.g. with hypothetical synchronous HTTP request functions, like you would have in other runtimes besides Node.js) we can continue.

Redsandro commented 11 years ago

In that case, I think I misunderstood you a few times, and I didn't realize until now. :(

If you mean what it was before I started playing with deferred, async and q, I can't show you literally because it is now mostly q and async. (Although I think I can replace async with q.)

But I can tell you, it was a big virtual noodle soup of events. I kept passing along an event emitter as the object bubbling through everything because it could emit events from everywhere. And I hooked what you'd concider the final then() to a listener in the first function. The promise chain has the benefit of not requiring this, because the scope of the enclosing function is available to all the anonymous functions in the chain.