NotifyTask concern

[sjb-sjb]

As I mentioned in another post, I'm a fan of NotifyTask. Recently however I noticed, well, not exactly a problem but something unexpected and I would say somewhat inconvenient. I would suggest a change.

The overall design of NotifyTask is that you hand the task in to the constructor, and then you await TaskCompleted. Meanwhile the properties of NotifyTask, such as IsCompleted, IsFaulted, ErrorMessage should be notified by NotifyTask.

But suppose the task being handed in to Create is already faulted? There are a lot of ways this could arise. What will happen is that MonitorTaskAsync will run to completion in the constructor, including NotifyProperties. But since we are still in the constructor of NotifyTask, there are no handlers attached to PropertyChanged at that point. Then when you await TaskCompleted, it returns immediately since the Task that it refers to has already run to completion. Consequently no property notifications will ever be sent.

In effect, the properties of NotifyTask are being initialized to values different from the default value (i.e. some of them are true while the default value for bool is false) but no property notifications occur. This is, I would say, somewhat logical but not at all convenient. We can't just do this:

    asyncExecution = NotifyTask.Create<TOut>( methodAsync() );
    asyncExecution.PropertyChanged += AsyncExecution_PropertyChanged;
    await asyncExecution.TaskCompleted; 

and define a handler

    private void AsyncExecution_PropertyChanged(object sender, PropertyChangedEventArgs e)
    {
        if (e.PropertyName == nameof(NotifyTask.InnerException)) {
            this.HandleException( ((NotifyTask)sender).InnerException);
        }
    }

Instead, in addition to doing all the above we also have to test for asyncExecution.InnerException != null after the call to Create. This basically leads to duplicating the logic of the PropertyChanged handler, which is not very desirable from a design point of view.

Also the fact that you have to do this is not very discoverable, as I discovered the hard way!

To remedy this situation my suggestion is to change MonitorTaskAsync as follows:

    private async Task MonitorTaskAsync(Task task)
    {
        try {
            if (task.IsCompleted) {
                await Task.Yield();
            }
            await task;
        } catch {
        } finally {
            NotifyProperties(task);
        }
    }

This way, MonitorTaskAsync will definitely not run to completion. Instead it will always return a meaningful Task that when awaited will do the appropriate NotifyProperties.

Note, this will give you a somewhat different flow of control from the current implementation. The Create call will now always yield while in the past it would only yield if the task was not already completed.

[StephenCleary]

I'm not sure that this would be an improvement.

This is the way that all updates work in an MVVM system; you have to read the property after subscribing to PropertyChanged. Every UI component works this way, and I fail to see a compelling reason why NotifyTask<T> should be different.

In particular, consider the situation where a completed task is passed to NotifyTask<T>. It would be truly weird for NotifyTask<T>.IsCompleted to return false if the task is clearly already complete.

On a side note, the proposed fix has a race condition that still allows MonitorTaskAsync to run (and invoke PropertyChanged) synchronously. You'd have to always do the Task.Yield to ensure consistent behavior.

[sjb-sjb]

Good comments!

I agree about the race condition, at least when the task is on a different thread. The task could complete between the time that we test for completion and the time that we await it. It is a mistake.

Whether the reason is "compelling" or not, I don't know. I agree that the original approach is logical. For me it is about convenience and reducing the duplication of code.

If we hand in a completed task then I believe that IsCompleted would be true but TaskCompleted.IsCompleted would be false. It seems OK to me that the task of checking for completion has not been completed, although the task itself has completed.

Thanks for your thoughts
sjb

[sjb-sjb]

Well .... I thought I understood this, but now I'm starting to wonder.

Let's say I have a method
async Task methodAsync() {...}
and I want to call back a function HandleException( Exception e) exactly once for each exception.

I set notifyTask = NotifyTask.Create( methodAsync()), then I hook up a property change handler. In the property change handler, when the Exception property changes then I call HandleException.

So far so good. But if the exception occurs before I hook up the property changed handler, then I am not notified. So I have to test for the exception by testing the properties of notifyTask. If I do this test before I set up the property changed handler then the exception can still occur after the test but before the property changed handler is set up -- in which case I miss the exception. On the other hand if I do it after I set up the property changed handler then the exception may occur after the handler is set up but before the test is done -- in which case I handle the exception twice instead of once?

Is a race condition inherent in using property change notifications with NotifyTask ?

In the end I would up doing something like this:

            abended = false;
            isExecuting = true;
            Task<TOut> task = null;
            try {
                task = methodAsync();
                await task;
            } catch (Exception) {
                foreach (Exception e in task.Exception.Flatten().InnerExceptions) {
                    HandleException(e);
                }
                abended = true;
            }
            isExecuting = false;
            TOut result = abended ? default(TOut) : task.Result;

[StephenCleary]

If you want to call HandleException exactly once for each exception, then you should do this:

notifyTask = NotifyTask.Create(async =>
{
  try { return await methodAsync(); }
  catch (Exception ex) { HandleException(ex); throw; }
});

[sjb-sjb]

I guess you are saying to do this but do not hook up PropertyChanged: since the exception is caught and passed directly to HandleException, there is no need to get it through the property-changed notification. I think this is somewhat similar to the suggestion I posted. My comment would be that we seem to be just cutting out the property-changed handler as a way of avoiding the race between the exception occurring and the property handler being hooked up.