Finally promise resolves even if chain is rejected about c-sharp-promise HOT 16 CLOSED

Hey, sorry I didn't see this response, or else I would have responded sooner. I'm not sure your example is quite right: even though the finally block on the inner try/finally gets called, your catch handler on the outer try/catch will still get called. E.g.

try {
    try {
        throw new Exception();
    } finally {
        Debug.Print("FINALLY");
    }
    Debug.Print("SUCCESS");
} catch (Exception ex) {
    Debug.Print("EXCEPTION");
}

Running this in a C# application will print:
FINALLY
EXCEPTION

In fact, if it didn't, finally blocks would be worthless because if you had to worry about finally swallowing an exception, you would just use a catch block and then code after the catch. I think the same behavior should be implemented in the Promise library: the promise returned from finally should be resolved if the chain above it is resolved, or rejected if the chain above it is rejected.

from c-sharp-promise.

That makes sense. Awesome, I'll get it done!

from c-sharp-promise.

Hmm yeah I can see how it would be useful. I do still think that because the functionality is different to finally we should think of a different name for them. Maybe "Always" like you suggested?

We use Semver for versioning, and because the change modifies the functionality of an existing method I'd probably consider that a breaking change.

I'm happy to sign the assembly - I'll create a key now and include the signed version next time I push a version to Nuget.

from c-sharp-promise.

I've renamed it "ContinueWith" now. Will be in the next release (probably 3.0)

from c-sharp-promise.

This is intended behaviour, although since finally isn't part of the A+ standard and isn't (yet) part of standard ECMAScript promises it's a bit less clear what it should do than some of the other methods.

The reason our implementation of finally is designed like this is because it more closely matches what we'd expect with regular try/catch/finally blocks. To illustrate this, here's a similarly structured example not using promises:

try 
{
    try 
    {
        throw new Exception();
    }
    finally 
    {
        // do nothing
    }
    Debug.Print("SUCCESS");
}
catch (Exception ex) 
{
    Debug.Print("FAILURE");
}

Because the finally block handles the exception, we can continue running the chain after it normally. If you want to catch an exception as well as use a finally block, you should put your catch handler before the finally:

new Promise().Then(() => {
    throw new Exception();
    })
    .Then(() => Debug.Print("SUCCESS")
    .Catch(ex =>  Debug.Print("FAILURE");
    .Finally(() => {});

In this case, the Debug.Print("SUCCESS") will be skipped if the promise before it is rejected, but the catch and finally after it will still be invoked.

from c-sharp-promise.

That's actually a good point. I checked the spec for the finally proposal again and it looks like you're right - a catch after a finally will still be called. I also checked the Bluebird and Q implementations of finally and they both behave in that way.

We generally try to keep this library compatible with JavaScript promises so if you'd like to have a go at implementing a version of finally that matches the spec in the TC39 proposal I'd be happy to merge that. It's currently a stage 3 proposal which means it's very close to becoming officially part of the standard. Apparently it's already avalable in the latest version of Chrome, Firefox beta, and Node.js via a command-line flag.

from c-sharp-promise.

Sure, I'll try and get to it later this week (gotta get some software shipped first). Mind reopening this issue and assigning it to me?

from c-sharp-promise.

Sure!

For reference, here's the JS code I used to test:

"use strict";

const Q = require("Q");
const bluebird = require("bluebird");

new bluebird((resolve, reject) => {
        console.log('Bluebird promise');
        reject(new Error('exception bluebird'));
    })
    .finally(() => console.log("Inside Bluebird finally"))
    .catch(err => console.log(err))
    .then(() => console.log("After error\n\n"));


Q().then(() => {
        console.log('Q promise');
        throw new Error("Q");
    })
    .finally(() => console.log('Inside Q finally'))
    .catch(err => console.log(err))
    .then(() => console.log("After error\n\n"));

In both those cases the log message for the finally is printed before the error from the catch.

Another interesting thing about the TC39 proposal is that values from a resolved promise before the finally are passed through as well as exceptions:

Q().then(() => {
        console.log('Q promise');
        return 'foo';
    })
    .finally(() => console.log('Inside finally'))
    .then(message => console.log(message));

/* prints: 
Q promise
Inside finally
foo
*/

from c-sharp-promise.

Hey sorry for the delay on this. I finally got around to implementing the changes to conform to the tc39 spec. Wasn't too bad actually, it was mostly there. One thing of note: I think the two Finally methods that take a Func (not an Action) should be renamed. I left these alone in my changeset (in that, the resulting promise will resolve even if the preceding promise rejects) because I think it's actually the right thing to do; the Finally code will resolve a promise, which will then add a new promise to the chain, and the chain then will resolve/reject based on that promise. But that doesn't really conform to the norms/expectations of try/catch/finally. The tc39 author, in a different context, mentioned considering the name Always for Finally; perhaps that makes more sense for these methods?

from c-sharp-promise.

Thanks! I took a look over your changes and it looks good. I'll check it against our larger codebase when I'm in the office tomorrow but so far looks pretty good.

As for the other version of "Finally" that takes a Func, I guess we could maybe mark it as obsolete now and remove it with the next major release or if we think it's useful maybe give it a different name? If it works in a different way to the new "Finally" it should at least have a different name, but we should probably also have a think about whether it's actually still necessary at all.

from c-sharp-promise.

I've merged your pull request and also marked the other versions of Finally as Obsolete.

Even though it's a small change I think it does have the potential to break existing code, so I'll have to do a major release (v3) before we can push it to Nuget. I figured since it's part of the standard that we're trying to track it was worth it though, and also issue #61 introduces some potentially breaking changes so I can fix that as well and bundle it into the same release.

from c-sharp-promise.

Awesome! Re: the versions of Finally that take a Func, I do use one of those variants as a way of buffering multiple calls to an asynchronous data loading function, in which I say "regardless of the outcome of the previous process, kick off a new process which may produce a new value". In this instance, it's equivalent to promise.Then(() => DoAThing()).Catch(_ => DoAThing()) but having that rolled into one method call (that is probably called something other than Finally) would still be convenient. I can share the specific code if you'd like (maybe there's a better way to do what I want to do with fewer needed API methods?)

Re: NuGet, would you consider a point release (e.g. 2.1) Also, and this may be a topic for another issue, but would you consider signing the NuGet package with a strong name? In my situation, I have to sign the app, which means all packages need to be signed; because of this, I don't currently use the NuGet package but I would much prefer doing so to compiling my own.

from c-sharp-promise.

FWIW, I borrowed "Always" from the tc39 readme (I guess there was early consideration of alternate names before they settled on finally). It would work though, I think. Other options that come to mind: AndThen, After, Chain/ChainWith, ContinueWith (this is borrowed from C# Tasks). Actually, ContinueWith is kind of my favorite of the bunch, and this usage mirrors that of the Task framework (a Task's ContinueWith is run after task completion, regardless of how it was completed).

from c-sharp-promise.

Is there a target release date for 3.0?

from c-sharp-promise.

Well you can grab the source now but I will probably push the new release to Nuget once issues #61 and #66 are fixed. I'm working on #61 (incorrect overloads of .Then and .Catch) now and #66 (custom exception types) has already partially been implemented in pull request #26 (.NET Core compat), but that needs some changes before it's ready to merge.

from c-sharp-promise.

If the .NET Core compat stuff takes a long time I might just implement the custom exception types separately. .NET Core compatibility is not a breaking change so could easily be added later. I want to make the exception types not inherit from ApplicationException, so that technically is a breaking change which is why I'm bundling it with the major release.

from c-sharp-promise.