Developing Open RIA Services Task-async API for the client  Viewed 63552 time(s), 6 post(s)., 7/11/2013 11:17:16 AM - by Daniel-Svensson

Daniel-Svensson
Daniel-Svensson 8/22/2013 1:04:42 PM

I've tried to post all relevant information from here at the Async group, i guess here are still some relevant info here which I've missed such as rationales for some features but we are better of discussing the feature over at the group from now on.

This content has not been rated yet. 
338 Reputation 44 Total posts
1
Daniel-Svensson
Daniel-Svensson 7/11/2013 11:17:16 AM

Background:

Some time ago I used the t4 client code generation to add task based overloads to our ria service projects, and we have started to almost exclusively use it since. The original design was based around some different examples for testable unit testable MVVM based clients (such as http://code.msdn.microsoft.com/Task-based-MVVM-Sample-for-2eb86fab) etc. And the concept works very well, and makes wonders (especially combined with async/await) when you are handling several parallel loads with more or less complex dependencies between them, but there are some changes I would like to make in order to have an API which conforms better with both how Task-sync APIs and how RIA servies works.

I would happily work with adding a Task-async API to the openriaservices client, and the intent of this post is to discuss the "design" of this async API.
As such I would like the propose a design similar to the following for adding Task-based async methods on the client.

Proposed API:

In addition to all the current function I suggest that we add Task-async "overloads" for Load, SubmitChanges and all Invoke operations.
The easy way forward for 4.3 would be to implement these over the current APM-based met

Current (APM) API

LoadOperation<T> Load(....)                                            
 
SubmitOperation SubmitChanges(...)
 
InvokeOperation<T> NameOfInvokeMehtod(...)


Task based API (this would be available, beside the current APM API):

Task<LoadResult<T>> LoadAsync<T>(...,  CancellationToken ct = ...)
 
Task<SubmitResult> SubmitChangesAsync() OR Task SubmitChangesAsync(... ,  CancellationToken ct = ...)
 
Task<T> NameOfInvokeMehtodAsync(...,  CancellationToken ct = ...)

As for the returned datatypes LoadResult and SubmitResult these would be based on the LoadOperation and SubmitOperation with the major difference that all properties related to error (exceptions) and are cancellation removed and that these are instead handled using Task's support for exceptions and cancellation. 

This in turns means that properties such as "EntitiesInError", and Validation errors which are only valid for failed requests would be added to  to DomainOperationException (or derived class/classes) instead.

In addition to (re)moving theses properties it would be really useful to have LoadResuilt<T> implement IEnumerable<T> (return the loaded entities) as well as ICollection. 
As for the invoke me hods I would model this the same way standard Task-based WCF methods would be generated with the same "return type" as on the server.

Below are some different Examples of how this would work in code:
Loading

/// APM approach
  ctx.Load(query, (loadOp) =>
  {
    if( loadOp.Exception != null)
    {
      // Handle exception
      loadOp.MarkErrorAsHandled();
      PrintErrorrs(loadOp.EntitiesInError);
    }
    else if (loadOp.IsCancelled)
    {
     // Handle cancellation
    }
    else
    {
       //Handle success
       list.Items = loadOp.Entities;
    }
    });
 
/// Task based approach
  ctx.LoadAsync(query).
    ContinueWith( (task) =>
    {
        if(task.IsFaulted != null)
        {
          // Handle exception, Note: no need to mark exception as handled just access exception
          PrintErrorrs( ((DomainOperationException)task.Exception).EntitiesInError);
         }
        else if (task.IsCancelled)
        {
         // Handle cancellation
        }
        else
        {
           //Handle success
           list.Items = task.Result; // or task.Result.Entities;
        }
    }, TaskScheduler.FromCurrentSyncronizationContext());
 
/// Task based approach with await
    try
    {
        lit.Items = await ctx.LoadAsync(query);
    }
    catch(TaskCancellationException tce)
    {
      // handle cancellation
    }
    catch(DomainOperationException doe)
    {
      // Handle exception, Note: no need to mark exception as handled just access exception
      PrintErrorrs(doe.EntitiesInError);
    }

Invoke operations:
// APM
_ctx.CalculateSum(20, 22, (res) =>
{
   if(res.Error != null) {
         res.MarkErrorAsHandled();
       // Handle error
       return ;
    }
    else
      this.Sum=res.Value;
});
 
// TPM with await
try
{
  this.Sum = await _ctx.CalculateSumAsync(20, 22);
}
catch(Exception ex)
{
// Handle error
}

So what do you think about adding this API to 4.3 and do anyone have any feedback regarding the design?

This content has not been rated yet. 
338 Reputation 44 Total posts
2
ColinBlair
ColinBlair 7/11/2013 1:21:40 PM

I have permission from Kyle McClellan to incorporate his original async code into Open RIA Services so we can use his code as a guide. http://blogs.msdn.com/b/kylemc/archive/2010/11/02/using-the-visual-studio-async-ctp-with-ria-services.aspx

I am thinking about taking things a step further then what you propose. The Load/Invoke/SubmitChanges methods have always been confusing because they are async but they didn't have that in their names. That was less of a problem on Silverlight where everything was async anyway, moving to other platforms I think it will be a problem. I am thinking that we should just drop the existing Load/Invoke/Submit methods and replace them with a new system designed for async completely. There may be a need for BeginLoad,BeginInvoke, and BeginSubmit that work like the old ones, but I don't think we need to build the new task based code on top of them.

For 4.3, I want to avoid any new external requirements so async will need to stay as extension methods instead of being built in. If someone wants to rewrite their code to use async then they should go ahead and move to the 5 branch completely.

OK, to the details now. I don't think I agree with stripping the error information out of the operations. The operation objects represent the state of the operation, if the operation had an error that is still part of the operation. If the operations didn't have an errors that is still something that the operation should know. What I would suggest instead is that the DomainOperationException should have a copy of the operation itself so that, even through the Task faulted, you can still get to the operation to find out what happened.

I am not sure about adding IEnumerable and ICollection directly to LoadOperation.

This content has not been rated yet. 
1539 Reputation 130 Total posts
3
Daniel-Svensson
Daniel-Svensson 7/29/2013 1:09:37 PM

It looks like i missed the rationale for my changes regarding error/cancellation for the task return types (see next post), but first some answers.

>> For 4.3, I want to avoid any new external requirements so async will need to stay as extension methods instead of being built in.
My implementation does not require any external dependencies, everything required is part of Silverlight 5 (we used our solution for 1-2 months before starting to use async/await). 
This way each user decide if they want to use the current API, the task based API without async or the task based API using async (this is the only option which requires the user to add dependency to the Microsoft.Bcl.Async package for his/hers code. Open Ria Services will not in itself have such a dependency)

>> I would suggest instead is that the DomainOperationException should have a copy of the operation itself so that, even through the Task faulted, you can still get to the operation to find out what happened.
That sounds like a good solution. The only downside I can see is that it is possible that users could expect to be able to get the "Operation" even for successful operations or to be able to specify UserState for the task-api.

 >> OK, to the details now. I don't think I agree with stripping the error information out of the operations.
I guess I was not clear enough on this part. I did not propose any change to any of the xxxOperations classes as I think these should stay the same. 
My idea was to have new "result" types which represent a successfully completed Load/Submit so once you have an instance of this return type the operation can never have been cancelled or have returned with an exception. While I don't think it is a good idea (especially not if we don't want to make it part of the public API that the Task-async API is initially built around the APM-Async API) it would definitely be possible to add an "Operation" member to these result types in order to make the Load/Submit-Operation accessible.

>> I am not sure about adding IEnumerable and ICollection directly to LoadOperation
Not sure if this was me being unclear with LoadResult vs LoadOperation or if you just wrote a bit fast. I only intendended this for LoadResult, not LoadOperation.
The main reason for implementing IEnumerable<T> is that at least on our code (and I guess it is quite common for other users of ria services) is that we load something which we immediately wan't to display or use and it would be useful to make this as easy as possible with as little code as possible.
 As for ICollection my reason is that if something is enumerable and have a Count property then it should also implement ICollection (non-generic variant).

>> I have permission from Kyle McClellan to incorporate his original async code into Open RIA Services so we can use his code as a guide. http://blogs.msdn.com/b/kylemc/archive/2010/11/02/using-the-visual-studio-async-ctp-with-ria-services.aspx

That is a good thing but I still think it is better to make the task's return a "result"-type instead of operation-types as it suffers from the same problems as the sample I posted (se explanation below).
In addition it is possible for the returned task to never complete due to a race-condition if the operation is completed on another thread. (but that should probably be quite easy to fix once the source code is available)

This content has not been rated yet. 
338 Reputation 44 Total posts
4
Daniel-Svensson
Daniel-Svensson 7/29/2013 3:28:59 PM

The main reason to have return types with out cancellation/error information from the Task-Async api is to only have one (correct) way to check for errors/cancellation instead of two where only one approach is correct.

 In our design our Task-Async functions return Task<ServiceLoadResult> where the ServiceLoadResult is quite similar  to one of Kyle's other posts (http://blogs.msdn.com/b/kylemc/archive/2011/04/29/mvvm-pattern-for-ria-services.aspx) and has properties for to access Error/Exception and Cancellation information (we had used our ServiceLoadResult class togheter with a callback based Load function for quite some time).
It was however a design flaw to use the same return type for both versions of the API since most developers at least at some time became confused about the correct way to write the code (despite code examples and a well-documentation "LoadAsync" method) and I think I have now seen all 4 combinations of error checking possible.

Even though I wrote the code myself I had to get back to look at the code in order to be 100% sure of the answer some weeks later when I got the question "Should I check the Error property on the Task, the ServiceLoadResult or both when I Load entities?". So far I have found several places in our code where only the ServiceLoadResult.Error was checked (which means that an unhandled exception was thrown when accessing the ServiceLoadResult using the Task.Result property or by awaiting the task), a few where both the task and the and ServiceLoadResult as checked (which is not an error in itself but the code could be much simplified by only checking it on the task) as well as few (in a single vm) with comments that it was unclear which property to check and that error handling was to be added later.

Since I strongly believe that an API should make it easy to do things right and difficult to get wrong (even when exploring the types using IntelliSense) it is a bad idea in my opinion to introduce the same flaw into Open Ria Services. I think that using two distinct return types ("result" and "operation") to distinguish a successfully completed operation from operations which might be in progress and which can potentially be cancelled could potentially prevent this category of bugs while at the same time making the API easier to learn.

Creating separate types which represents a successfully completed operation which looks like SubmitOperation and LoadOperation but without the unnecessary/confusing would leave us with something similar to the following "psuedo"-classes. 

01.// This is approximately how the SubmitResult could look like
02.public class SubmitResult
03.{
04.     public EntityChangeSet ChangeSet {get; }
05.}
06. 
07.// This is approximately how the LoadResult could look like
08.public class LoadResult<TEntity> : IEnumerable<TEntity>, ICollection
09.{
10.   private SomeCollectionType<TEntity> _loadedEntites;
11. 
12.   public IEnumerable<TEntity> Entities { get { return _loadedEntites; } }
13.   public IEnumerable<Entity> AllEntities {get;}
14.   public EntityQuery<TEntity> EntityQuery {get; }
15. 
16.   public int TotalEntityCount {get;}
17. 
18.   public LoadBehavior LoadBehavior {get;}
19. 
20.   // implements ICollection.Count
21.   public Count { return _loadedEntites.Count; }
22. 
23.   IEnumerator<T> IEnumerable.GetEnumerator() { return _loadedEntities;}
24.}


As for classes vs interfaces discussions we could consider interfaces like these (but where the ICollection/IEnumerable part is removed for the "ILoadResult") which could then be implemented both by the Submit/LoadOperation as well as the Submit/LoadResult classes. But that is not something i would add unless it is a very strong support for it in order to be able to handle the result from both TaskAsync functions and the current (and future BegingXXX) async-API by the same function and then you would still need two different implementations for handling errors/cancellation.

This content has not been rated yet. 
338 Reputation 44 Total posts
5
ColinBlair
ColinBlair 8/5/2013 9:20:51 PM

Sorry, I was on vacation last week. One thing I do not like dealing with is complicated subjects in a single message thread. It makes conversation difficult, it lets important points slide off the side that should be. So, when I see something like Async support which looks like something  that can complicate things that way I will open up new groups to specifically discuss the subject. That gives us a dedicated message board where we can split things up into multiple message threads as needed. For example, the topic of 4.3 support is something that should be a separate thread. I will probably ignore that thread until we are much further along in the design process. I have to be careful to protect 4.3, so we are all much better off just talking about how to design async for 5 and then once we are far along revisit 4.3.

I am going to add a few posts in the Async group with what I am currently thinking. I would like to continue the conversation there.

EDIT: After putting in what I was thinking, I think I am coming more in line with your thoughts Daniel so I have deleted what I posted. What I would like you to do is post three messages, Load, Invoke, and Submit with what you think the code should look like and how it would be used. I will tell you that the one I am having the most trouble with is Submit. The only reason I need the ChangeSet after a Submit is if the submit failed.

This content has not been rated yet. 
1539 Reputation 130 Total posts
6