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

Applying the "try-catch-finally"/"using" patterns #9

Closed
zsoi opened this issue Oct 9, 2015 · 7 comments
Closed

Applying the "try-catch-finally"/"using" patterns #9

zsoi opened this issue Oct 9, 2015 · 7 comments

Comments

@zsoi
Copy link
Contributor

zsoi commented Oct 9, 2015

I started using Promises to model asynchronous/pipelined execution flows in our game and I'm really impressed, it just works as I would expect! As I use it e.g. with I/O, I'd like to start a discussion on how to extend the library to work better with C# constructs like "using" and "try-catch-finally".

Try/Catch works (but the Catch method is violating A+ as it cannot return a value/promise) but I struggle to create a good "finally" case. Consider this:

public IPromise Store() 
{
    return Promise.Resolved().Then(() =>
    {
        memoryStream ms = new MemoryStream();
        return StoreImpl(ms);
    });
}

private IPromise StoreImpl(MemoryStream ms)
{
    return Promise.Resolved().Then(() => 
    {
        throw new Exception("That did not work as expected");
    });
}

How would Store() dispose of the MemoryStream when an exception occurs? A Then() callback would not suffice, it would work when the preceding promise resolves, but when it rejects it won't. The disposing code could be duplicated and added as Then() and Catch() callbacks, but by specifying a rejection callback at this point, the exception will not be passed to the client-side error handling. I could explicitely re-throw the exception in a Catch() callback, but that would not preserve the exception per se. The only way I could think of would be to duplicate the disposal code and return another rejecting promise with the same exception, which is currently not possible because the Catch() callbacks can not return a promise (violating the A+ spec):

IPromise Store() 
{
    return Promise.Resolved().Then(() =>
    {
        memoryStream ms = new MemoryStream();

        return StoreImpl(ms)
            .Catch((ex) => {
                ms.Dispose();
                // Currently cannot do this, the rejection handler can not return values but it should
                return Promise.Reject(ex);
            })
            .Then(() => {
                ms.Dispose();
                return Promise.Resolved();
            });
    });
}

What do you think about this? I'll maybe try to fork and fix the Catch() handling myself, but I'd also like to get some feedback on this issue first. Also I'd like to know if there are any other suggestions how finally can be supported more fluently and maybe even how using could be supported in any way!

Suggestion for finally:

  • Implement a IPromise Promise.Finally(Func<IPromise> callback) instance method
  • The callback type of finally does not take any parameters (neither value nor exception!)
  • The Finally callback can return a new promise
  • Finally itself returns a promise
  • The finally callback just sits transparently in the flow of execution, all values/exceptions are passed "around" it and given to the chained handlers

Suggestion for using:

  • Implement static method IPromise Promise.Using(IDisposable, Func<IPromise>)
  • The IDisposable is disposed off when the promise returned by the callback is resolved/rejected, via the new Finally() interface

Basically implemented like this:

static IPromise Using(IDisposable d, Func<IPromise> cb)
{
    return Promise.Resolved().Then(() => {
        return cb();
    }).Finally(() => {
        d.Dispose();
    });
}

I'd love to see this library going forward, as we won't get 4.5 features in unity in the near future and this is a very good alternative!

@ashleydavis
Copy link
Contributor

This an interesting discussion. Can I just clarify one thing... it seems to me that in this case you could just do this:

public IPromise Store() 
{
    return Promise.Resolved().Then(() =>
    {
        using (memoryStream ms = new MemoryStream()) 
        {
            return StoreImpl(ms);
        }
    });
}

private IPromise StoreImpl(MemoryStream ms)
{
    return Promise.Resolved().Then(() => 
    {
        throw new Exception("That did not work as expected");
    });
}

@ashleydavis
Copy link
Contributor

For your issue with Catch, can you just do this:

IPromise Store() 
{
    return Promise.Resolved().Then(() =>
    {
        memoryStream ms = new MemoryStream();

        return StoreImpl(ms)
            .Catch((ex) => {
                ms.Dispose();
                throw ex;                
            })
            .Then(() => {
                ms.Dispose();
                return Promise.Resolved();
            });
    });
}

@ashleydavis
Copy link
Contributor

I'm interested in your ideas for the Finally and Using functions. Can you please make up some examples of use?

@zsoi
Copy link
Contributor Author

zsoi commented Oct 18, 2015

In the specific case that I stated yes, thats true, I could just encapsulate it into a using block. But let's assume that we're really resolving the promises asynchronously (Actually it is really important that promises get always resolved asynchronously, one would need a "platform" layer for that, but that's a different topic) then the Stream would be disposed before the resolve handler is called which uses the Stream.

One use case for "Finally" would be to e.g. unlock the UI after a asynchronous operation:

IPromise<string> RequestFromServer(string url)
{
    LockUI();

    return WWWDownload(url).Finally(() => UnlockUI());
}

void Act()
{
    RequestFromServer("http://...").Then((s) => {
        OpenPopup("Answer: " + s);
    }).Catch((x) => {
        OpenPopup("Error: " + x.Message);
    });
}

The "Using" would just be some syntactical sugar on top of the try-catch-finally-API of the promise but would clean up boilerplate code:

IPromise<string> LoadFromFileNoUsing(string path)
{
    FileStream fs = new FileStream(path);

    return ReadAsyncFromFile(fs).Finally( () => {
            ((IDisposable)fs).Dispose();
    });
}

IPromise<string> LoadFromFile(string path)
{
    FileStream fs = new FileStream(path);

    return Promise.Use(fs, () => {
        return ReadAsyncFromFile(fs);
    };
}

IPromise<string> ReadAsyncFromFile(FileStream fs)
{
    // ...
}

@ashleydavis
Copy link
Contributor

I love both of these ideas.

Please feel free to contribute and continue the conversation here. If you are adding features we just ask that you keep the existing tests working and add unit-tests for any new features (ideally via TDD).

@zakyg
Copy link

zakyg commented Jan 17, 2016

Is the finally pattern implemented yet? We have something that needs to be done whether a chain of promises succeeds or fails. It's pretty standard in Promise implementations. Here's the example from bluebird:
http://bluebirdjs.com/docs/api/finally.html

@ashleydavis
Copy link
Contributor

We really want the finally feature but have little time to implement it at the moment. Please feel free to fork and contribute.

BrknRobot added a commit to BrknRobot/C-Sharp-Promise that referenced this issue Jun 24, 2016
@BrknRobot BrknRobot mentioned this issue Jun 24, 2016
BrknRobot added a commit to BrknRobot/C-Sharp-Promise that referenced this issue Mar 17, 2017
Implemented as described in issue Real-Serious-Games#9. It's just a wrapper around existing functionality.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants