views:

1435

answers:

6

Hello,

C# 2008

I am using the code below to login to a softphone. However, the login progess is a long process as there are many things that have to be initialized and checks to be made, I have only put a few on here, as it would make the code to long to post.

In the code below I am checking if the CancellationPending if the CancelAsync has been called in my cancel button click event, before doing each check. Is this correct? Also if the check fails I also call the CancelAsync and set the e.Cancel to true.

I would like to know if my method I have used here is the best method to use.

Many thanks for any advice,

private void bgwProcessLogin_DoWork(object sender, DoWorkEventArgs e)
    {   
        /*
         * Perform at test to see if the background worker has been
         * cancelled by the user before attemping to continue to login.
         * 
         * Cancel background worker on any failed attemp to login
         */

        // Start with cancel being false as to reset this if cancel has been set to true
        // in the cancel button.
        e.Cancel = false;

        NetworkingTest connection_test = new NetworkingTest();
        if (!this.bgwProcessLogin.CancellationPending)
        { 
            // Check local LAN or Wireless connection               
            if (!connection_test.IsNetworkConnected())
            {
                // Update label
                if (this.lblRegistering.InvokeRequired)
                {
                    this.lblRegistering.Invoke(new UpdateRegisterLabelDelegate(UpdateRegisterLabel), "No network connection");
                }
                else
                {
                    this.lblRegistering.Text = "No network connection";
                }
                // Failed attemp
                this.bgwProcessLogin.CancelAsync();
                e.Cancel = true;
                return;
            }
            // Report current progress
            this.bgwProcessLogin.ReportProgress(0, "Network connected");
        }
        else
        {
            // User cancelled 
            e.Cancel = true;
            return;
        }

        // Test if access to Server is available
        if (!this.bgwProcessLogin.CancellationPending)
        {
            if (!connection_test.IsSIPServerAvailable())
            {
                // Update label
                if (this.lblRegistering.InvokeRequired)
                {
                    this.lblRegistering.Invoke(new UpdateRegisterLabelDelegate(UpdateRegisterLabel), "Server unavailable");
                }
                else
                {
                    this.lblRegistering.Text = "Server unavailable";
                }
                // Failed attemp
                this.bgwProcessLogin.CancelAsync();
                e.Cancel = true;
                return;
            }
            // Report current progress
            this.bgwProcessLogin.ReportProgress(1, "Server available");
        }
        else
        {
            // User cancelled 
            e.Cancel = true;
            return;
        }
        .
        .
        .
}


 private void bgwProcessLogin_RunWorkerCompleted(object sender, RunWorkerCompletedEventArgs e)
    {   
        // Check for any errors
        if (e.Error == null)
        {
            if (e.Cancelled)
            {
                // User cancelled login or login failed                
            }
            else
            {
                // Login completed successfully                
            }
        }
        else
        {
            // Something failed display error
            this.statusDisplay1.CallStatus = e.Error.Message;
        }
    }


 private void bgwProcessLogin_ProgressChanged(object sender, ProgressChangedEventArgs e)
    {
        this.lblRegistering.Text = e.UserState.ToString();
    }

private void btnCancel_Click(object sender, EventArgs e)
    {
        // Cancel the logging in process
        this.bgwProcessLogin.CancelAsync();
        this.lblRegistering.Text = "Logged out";
}
+1  A: 

Looks good.

Vixen
+1  A: 

Looks pretty good to me also.

Lucas B
+1  A: 

You are doing it the right way, I believe. You will find thread members that allow you to terminate or abort a thread, but you don't want to use them for something like this. It might look a little weird to have all of the "cancelled" checks in your code, but that allows you to control exactly when you exit your thread. If you were to "rudely" abort the worker thread, the thread has no control of when it exits, and there could be corrupted state.

JMarsch
A: 

Hello,

There is one thing I don't need to call the this.bgwProcessLogin.CancelAsync(); as you can just set this e.Cancel = true;

robUK
+1  A: 

Within your DoWork() function you wrote .... Depending on how many tasks of the same structure are coming like the displayed two one, you could refactor this structure into an own method, giving the changing parts as parameters.

Also this InvokeRequired if-else branch has doubled the output string. A little search here on stackoverflow or on the web should show you a pattern to accomplish this doubling.

Evernything else looks quite good.

Oliver
+1  A: 

There is maybe only one problem: if one of the operation in DoWork event handler would last for a long time. In this case you could abort your pending operation ONLY after that operation finished. If all operations in DoWork event can't last very long (for instance, no more than 5 seconds), its all OK, but if one of the operations can last for long time (5 minutes, for instance) in this case user have to wait until this operation finished.

If DoWork contains long lasting operations you can use something like AbortableBackgroundWorker. Something like this:

public class AbortableBackgroundWorker : BackgroundWorker
{
    private Thread workerThread;

    protected override void OnDoWork(DoWorkEventArgs e)
    {
        workerThread = Thread.CurrentThread;
        try
        {
            base.OnDoWork(e);
        }
        catch (ThreadAbortException)
        {
            e.Cancel = true; //We must set Cancel property to true!
            Thread.ResetAbort(); //Prevents ThreadAbortException propagation
        }
    }


    public void Abort()
    {
        if (workerThread != null)
        {
            workerThread.Abort();
            workerThread = null;
        }
    }
}

In this case you can truly abort pending operations, but you also have some restrictions (for more information about aborting managed thread and some restrictions see Plumbing the Depths of the ThreadAbortException Using Rotor).

P.S. I agree with Oliver that you should wrap InvokeRequired in more usable form.

Sergey Teplyakov