MarimerLLC / cslaforum

Discussion forum for CSLA .NET
https://cslanet.com
Other
31 stars 6 forks source link

DataPortal.FetchChildAsync missing #522

Open j055 opened 6 years ago

j055 commented 6 years ago

Hi

Is there a reason CSLA doesn't not have DataPortal.FetchChildAsync to complement DataPortal.FetchAsync? I can't see a way to use async\await pattern all the way down to a child object. Is there a solution/pattern I should apply?

    public static async Task<BusObj> GetAsync()
    {
        return await DataPortal.FetchAsync<BusObj>();
    }

    private async Task DataPortal_Fetch()
    {
        RaiseListChangedEvents = false;
        IsReadOnly = false;

        foreach (var item in items)
        {
            // how do we use async/await with child objects
            Add(await DataPortal.FetchChildAsync<ChildBusObj>());
        }

        IsReadOnly = true;
        RaiseListChangedEvents = true;
    }
jonnybee commented 6 years ago

In a typical patent-child I would the child data in the parent DataPortal_Fetch and pass the data reader or list as a parameter to the child/child list.

rockfordlhotka commented 6 years ago

It is also the case that CSLA isn't threadsafe, at least within the context of having multiple threads update the object graph simultaneously.

So we perhaps could support async/await from a database IO perspective, because that's all actually running on a single thread. But as soon as async/await is allowed I will bet that someone will use a Parallel.ForEach or something, resulting in multiple threads trying to update the graph at the same time and causing total chaos/disaster.

ngm011 commented 5 years ago

While async-fied Child_Fetch might not be that needed, I think async version of Child_Insert/Update would be much welcomed.

For instance, what's the recommended way of obtaining identity values (DB generated ID's) while persisting down the child graph? Some other async interaction points can be easily envisioned during child persistence.

As for Parallel.ForEach, clueless dev might as well use Task.Start :)

rockfordlhotka commented 5 years ago

You can mark Child_Fetch (for example) as async void.

CSLA will invoke it synchronously, but inside the method you can use IO based async operations with await.

You are right @ngm011, that if someone spins up a thread in that method they'll be in trouble. But that's a whole different thing than using await on an IO blocking operation like talking to a database.

In the years this model has been in place I have yet to hear of anyone encountering a problem. I suspect that's because people do data access in these methods (thus IO blocking), and it is hard to imagine a scenario where someone would want to mix in some background computing task as part of their data layer.

ngm011 commented 5 years ago

@rockfordlhotka, I don't think we're on the same page here.

I'm not talking about background operation at all and I'm aware that Child_XXX methods can have awaitable calls inside but they cannot be awaited i.e. Data Portal is not going to await their invocation.

Essentially, that means the children persistence cannot be coordinated which I thought is the whole point of portal's persistence invocations. Let's say child B depends on the child A to be persisted first for the reason as simple as getting child A's newly generated ID. Also, it can potentially have some side effects as well, such as root with DataPortal_Insert/Update might close the connection/DB Context/transaction prior the child's completion of its I/O op that relies on the same resource.

Unless I'm missing something here, this looks pretty vital to me.

rockfordlhotka commented 5 years ago

You are right - my mind went off somewhere I guess...

I believe that this issue will be addressed in CSLA 5. I have everything else updated except the child data portal at this point actually - with the primary goals being to support dependency injection and the ability to name your data portal methods whatever you want.

My plan is to use the same invocation model I've built for the DI stuff (which supports await too) in the child data portal, just like in the root data portal.

This (pretty big) PR (https://github.com/MarimerLLC/csla/pull/1163) is where the work is occurring. Though it is probably simpler to look at the working branch than all the diffs (https://github.com/rockfordlhotka/csla/tree/1102-dataportal).

You can see how the DI and async calling mechanisms are merged in the new DataPortalTarget class.

ngm011 commented 5 years ago

Huh, it is big indeed. Great job though.

I really like refactoring accomplished with DataPortalTarget - it eliminates so much redundancy in data portal methods and keeps their intention way clearer.

As for ServiceProviderMethodCaller, there's so much ceremony to just choose the method to be invoked it's not even funny. Are people so religious about naming these things? Even though I was never big fan of "DI hysteria" (I know you weren't too) it's definitely good addition. I just wonder if infamous IServiceProvider will be able to accommodate more complex DI scenarios/containers.

Aside from this, I'm struggling to understand ThrowIfAsyncMethodOnSyncClient guard. Is it related to potential deadlock if client is waiting on the same context the DP is going to try to complete on?

rockfordlhotka commented 5 years ago

The use of isSync is required because of synchronization context issues with some UI technologies, where (as you say) there are deadlock issues due to the sync context funneling everything back to a "UI thread".

Rather than leaving the code unguarded and letting people (you) deal with hard-to-debug deadlocks, I chose to have CSLA explicitly throw an exception in the case that a deadlock is likely - thus making it easy to identify scenarios that just can't work.

ngm011 commented 5 years ago

Good stuff, thanks for confirming it.