Async insecurities

Question

I feel like I'm overdoing the async await stuff. I'm just learning. Maybe this isn't the best instance to be using this stuff.

I have a system with sensors that detect the completion of some action. I decided to approach this solution using asynchronous programming since I still need to communicate with other controllers as well as track the progress in changing states. Here is a snippet of the mess I made.

Is this a good instance to use this or should I revert back to tasks and events?

 private async void StartTest_Click(object sender, RoutedEventArgs e)
        {


            if (AdamCont == null)
            { return; }


            Abort = new CancellationTokenSource();

            int reached = await AdamCont.RunTestConfigReached(Abort.Token);

            if (reached == 0)
            {
                LightControl(SignalLight.GreenLight);                   
                Model.TestState = TestCycleState.NotInTest;
            }
            else if (reached > 0)
            {
                LightControl(SignalLight.RedLight);                   
                Model.TestState = TestCycleState.NotInTest;
               // OnFailed(this, new FailedPositionEventArgs(Model.TestState, String.Format("Step {0} in test configuration failed", reached)));
                //Now failedPosition can be called directly.
                FailedPosition( new FailedPositionEventArgs(Model.TestState, String.Format("Step {0} in test configuration failed", reached)));
            }
            else
            {
                LightControl(SignalLight.AmberLight);                   
                Model.TestState = TestCycleState.NotInTest;
            }

        }

The call is waiting on this sequence of operations.

    public async Task<int> RunTestConfigReached(CancellationToken abortToken)
            {
                //extend seal bar
                this.SetDIOState(true, DOChannels_5045_1.SEAL_CLAMP_DOWN);
                this.SetDIOState(false, DOChannels_5045_1.SEAL_CLAMP_UP);
                this.SetDIOState(false, DOChannels_5045_1.SEAL_BAR_CYLINDER_IN);
                this.SetDIOState(true, DOChannels_5045_1.SEAL_BAR_CYLINDER_OUT);
                this.LightControl(SignalLight.AmberLight);

                Model.TestState = TestCycleState.SealBarLoadPosition;
                bool reached = await Task<bool>.Factory.StartNew(() => SealBarLoadPosReached(200, 100));

                if(!reached)
                {
                   return 1;
                }

            if(abortToken.IsCancellationRequested)
            {
                return -1;
            }


            // Retract seal bar
            this.SetDIOState(true, DOChannels_5045_1.SEAL_BAR_CYLINDER_IN);
            this.SetDIOState(false, DOChannels_5045_1.SEAL_BAR_CYLINDER_OUT);
            Model.TestState = TestCycleState.SealBarRetractPosition;

            reached = await Task<bool>.Factory.StartNew(() => SealBarRetractPosReached(200, 100));

            if(!reached)
            {
                return 2;
            }

            if (abortToken.IsCancellationRequested)
            {
                return -1;
            }

   //... etc...
    }

RunTestConfigReached() is waiting on individual states of the sensors. An example is:

private bool SealBarLoadPosReached(int timeout, int integrationTime = 200)
        {
             bool configurationReached = false;
            int counter = 0;
            while (counter < timeout && !configurationReached)
            {
                configurationReached =!Model.ChannelStates_5045_1[(int)DIChannels_5045_1.SEAL_BAR_HOME]
                                        && !Model.ChannelStates_5045_1[(int)DIChannels_5045_1.SEAL_PICUP_POSITION]  //means it is
                                        && Model.ChannelStates_5045_1[(int)DIChannels_5045_1.SEAL_TEST_POSITION]    // means it is not
                                        && Model.ChannelStates_5045_1[(int)DIChannels_5045_1.RIGHT_CLAMP_DOWN]
                                        && !Model.ChannelStates_5045_1[(int)DIChannels_5045_1.RIGHT_CLAMP_UP]
                                        && Model.ChannelStates_5045_1[(int)DIChannels_5045_1.LEFT_CLAMP_DOWN]
                                        && !Model.ChannelStates_5045_1[(int)DIChannels_5045_1.LEFT_CLAMP_UP];
                Thread.Sleep(integrationTime);
                counter++;
            }
            return configurationReached;
        }

Answer 1

It would help to see how the UI is calling this because then we can see if UI blocking concerns are mitigated, but since you might get scolded for changing your question now that I've answered, I'm going to assume the button click event is calling everything properly allowing for asynchronous UI updates.

It's difficult to try and define a problem up front that asynchronous programming will solve for your app, but I can tell you it's a good fit. It may be helpful to take the async programming level up a notch and then see what it's buying you. You're only dealing with one process here for now--instead of overdoing it, I'd say you're underdoing it!

I'd start with trying to eliminate immediately awaiting every task you start up. This is making your process essentially run synchronously. Once you start returning task objects from methods and awaiting task objects outside of the methods you started the tasks in, it'll probably start clicking.

If playing around with it is getting you nowhere, try focusing on another event that needs to run while the workflow you've posted is running. It'll probably help once you start managing 2 things happening at the same time.

I think the area of your app that may benefit the most from asynchronous programming is actually an area of code where you haven't utilized it--the SealBarLoadPosReached method. Since you're no longer operating on the main UI thread, this isn't causing blocking issues, but it is tying up a thread unnecessarily. Instead of a while loop that calls Thread.Sleep, blocking the thread, you can make your method async and await Task.Delay(milliseconds) so that the thread is freed up until the time is up.

Asynchronous programming goes beyond just avoiding UI thread blocking. For example, consider what would happen if you removed the await before your SealBarLoadPosReached method call if you made it asynchronous and started dealing with a local Task. When your newly-added await Task.Delay line is hit, the remainder of RunTestConfigReached will immediately execute. Obviously this isn't the behavior you'd like, but you can take advantage of this. For example, you can move the result inspection logic into a new (or anonymous) method, and then call ContinueWith on the local task, providing that method, so that when the task completes, that method is invoked and the result of the task passed into it. This can help eliminate race conditions provided you don't modify too much global state in these "callbacks." This would be especially helpful if you wanted to call this the inspection logic/event trigger from other places.

I can't say for sure that what I just suggested is appropriate for your app, but I do recommend trying some variant of it or just mixing it up a bit.

At the very least it will highlight some items that potentially need solving in your app already. As your app grows in complexity, continuing to imperatively command things to wait n milliseconds may lead to race conditions, whether you decide to use Task.Delay() or Thread.Sleep(). If you do start using ContinueWith, do you want to call ContinueWith timeout number of times to set up the continuations? The questions abound!

I'd also recommend attempting to completely remove EventArgs and seeing what your code looks like after you change it to depend on task results. (EventArgs can become request/response blobs of data that quickly fill up with one-off properties). Instead of imperatively calling events like LightControl, try setting up LightControl so that it's called based on a task completing and the result (and not by just calling .Wait() on a task to get the result!). A common pattern may emerge, creating a go-between that eliminates the need to call UI events from core logic.

Like I said before, it'll probably start to fall into place the more you play around with it.

Answer 2

The first thing that needs to be addressed is the indentation and the spacing, because that's the real issue here.

You should line your braces with the start of the line above it, like so :

if()
{
}

Instead of :

 if(reached == 0)
   {
    // Yay it completed.
   }

While we're at it, this code is weird :

if(reached == 0)
{
    // Yay it completed.
}
else if(reached > 0)
{
    // Boo it didn't.
    OnFailed(this, new FailedPositionEventArgs(Model.TestState, String.Format("Step {0} in test configuration failed",reached)));
}

What happens when reached is below zero? Did it completed or not? Either way, have an empty content for your if is weird, you should remove it, since you do nothing in it.

if(reached > 0)
{
    //Didn't succeed.
    OnFailed(this, new FailedPositionEventArgs(Model.TestState, String.Format("Step {0} in test configuration failed",reached)));
}

async void is usually bad. Consider returning a async Task, because otherwise the one that launches the async task won't be able to "follow" it. If the "launcher" doesn't want to follow it, that's his problem, but you must give him the opportunity to do so.

Instead of :

if (reached == 0)
{
    LightControl(SignalLight.GreenLight);                   
    Model.TestState = TestCycleState.NotInTest;
}
else if (reached > 0)
{
    LightControl(SignalLight.RedLight);                   
    Model.TestState = TestCycleState.NotInTest;
   // OnFailed(this, new FailedPositionEventArgs(Model.TestState, String.Format("Step {0} in test configuration failed", reached)));
    //Now failedPosition can be called directly.
    FailedPosition( new FailedPositionEventArgs(Model.TestState, String.Format("Step {0} in test configuration failed", reached)));
}
else
{
    LightControl(SignalLight.AmberLight);                   
    Model.TestState = TestCycleState.NotInTest;
}

Why don't you just do :

if (reached <= 0)
{
    LightControl(SignalLight.GreenLight);                   
    Model.TestState = TestCycleState.NotInTest;
}
else
{
    LightControl(SignalLight.RedLight);                   
    Model.TestState = TestCycleState.NotInTest;
   // OnFailed(this, new FailedPositionEventArgs(Model.TestState, String.Format("Step {0} in test configuration failed", reached)));
    //Now failedPosition can be called directly.
    FailedPosition( new FailedPositionEventArgs(Model.TestState, String.Format("Step {0} in test configuration failed", reached)));
}

It means the same thing, there's no code duplication and less conditional instruction if/else.

Answer 3

As per async / await best practice, you should never use async void. You should modify your private async void StartTest to private async Task StartTest. Here is the MSDN best practice link.