views:

2822

answers:

8

I am relatively new to C#/.Net. I'm developing a desktop application that requires multi threading. I came up with the following pattern below as a base. I was wondering if anyone could point out how to make it better in terms of coding, being thread safe, and being efficient.

Hopefully this makes some sense.


public abstract class ThreadManagerBase
{
    // static class variables

    private static ThreadManagerBase instance = null;
    private static BackgroundWorker thread = null;
    private static ProgressBarUIForm progress = null;

    /// <summary>
    /// Create a new instance of this class. The internals are left to the derived class to figure out.
    /// Only one instance of this can run at any time. There should only be the main thread and this thread.
    /// </summary>
    public abstract static ThreadManagerBase NewInstance();

    /// <summary>
    /// Clears the instance.
    /// </summary>
    public static void ClearInstance()
    {
        instance = null;
    }

    /// <summary>
    /// Initializes the background worker with some presets.
    /// Displays progress bar.
    /// </summary>
    private abstract static void InitializeThread()
    {
        thread = new BackgroundWorker();

        thread.WorkerReportsProgress = true;
        thread.WorkerSupportsCancellation = true;

        thread.DoWork += new DoWorkEventHandler(thread_DoWork);
        thread.RunWorkerCompleted += new RunWorkerCompletedEventHandler(thread_RunWorkerCompleted);
        thread.ProgressChanged += new ProgressChangedEventHandler(thread_ProgressChanged);

        thread.RunWorkerAsync();

        progress = new ProgressBarUIForm();

        progress.EnableCancelButton = true;

        progress.UserCanceled += new EventHandlerCancelClicked(progress_UserCanceled);

        progress.ShowDialog();

        thread.Dispose();

        thread = null;
    }

    private static void progress_UserCanceled(bool userCanceled)
    {
        thread.CancelAsync();
    }

    private static void thread_ProgressChanged(object sender, ProgressChangedEventArgs e)
    {
        progress.SetProgressLevel = e.ProgressPercentage;
        progress.SetProgressMessage = e.UserState.ToString();
    }

    private static void thread_RunWorkerCompleted(object sender, RunWorkerCompletedEventArgs e)
    {
        progress.Close();

        progress = null;
    }

    private static void thread_DoWork(object sender, DoWorkEventArgs e)
    {
        ProcessWork();
    }

    private abstract static void ProcessWork()
    {
        // do actuall stuff here.
        // the derived classes will take care of the plumbing.
    }
}
+1  A: 

You don't need a BackgroundWorker unless you want to be spoonfed, normal threads are perfectly acceptable, as long as you follow the rules.

leppie
What "rules" are you referring too?
Brownman98
I guess you could follow Microsoft's guide to Threading. http://msdn.microsoft.com/en-us/library/e1dx6b2h.aspx Should be right ;)
alphabeat
What's wrong with being spoon-fed.
John Kraft
A: 

I don't see a good reason to create this abstraction over BackgroundWorker. If you insist, just a warning: I'm not sure if it changed in later releases, but in NET 2.0, it wasn't possible to really cancel the DoWork handler (unless it checked once in a while if it was asked to stop). Read here for a solution.

muratgu
+1  A: 

Have you looked into the Microsoft Parallel Extensions to .NET Framework 3.5? It's a pretty good library that takes a lot of the work out of threading.

There are also a lot of articles on MSDN about threading patterns that you should research too. Threading can get really complicated really fast. It's nice to have someone else to have though of all the important stuff that can go wrong, and simplify it down to a library or a pattern. Of course, there's danger in that too if you don't understand the gotchas of any particular solution. So, make sure you research well whatever solution you choose.

Mufasa
A: 

I have done something similar to this. There is a good reason if you do have multiple tasks that you want to perform, but you dont want to have BackgroundWorker code replicated through the entire project. I dont have the progressbar tied to the actual base class, I just have that in the main form. Here is the solution I came up with:

The following is the base class:


   public abstract class Operation
   {
      #region public Event Handlers

      /// 
      /// The event that updates the progress of the operation
      /// 
      public event OperationProgressChangedEventHandler OperationProgressChanged;

      /// 
      /// The event that notifies that the operation is complete (and results)
      /// 
      public event OperationCompletedEventHandler OperationCompleted;

      #endregion

      #region Members

      // Whether or not we can cancel the operation
      private bool mWorkerSupportsCancellation = false;
      // The task worker that handles running the operation
      private BackgroundWorker mOperationWorker;
      // The operation parameters
      private object[] mOperationParameters;

      #endregion

      /// 
      /// Base class for all operations
      /// 
      public Operation(params object[] workerParameters)
      {
         mOperationParameters = workerParameters;
         // Setup the worker
         SetupOperationWorker();
      }

      #region Setup Functions

      /// 
      /// Setup the background worker to run our Operations
      /// 
      private void SetupOperationWorker()
      {
         mOperationWorker = new BackgroundWorker();
         mOperationWorker.WorkerSupportsCancellation = mWorkerSupportsCancellation;
         mOperationWorker.WorkerReportsProgress = true;
         mOperationWorker.WorkerSupportsCancellation = true;
         mOperationWorker.DoWork += new DoWorkEventHandler(OperationWorkerDoWork);
         mOperationWorker.ProgressChanged += new ProgressChangedEventHandler(OperationWorkerProgressChanged);
         mOperationWorker.RunWorkerCompleted += new RunWorkerCompletedEventHandler(OperationWorkerRunWorkerCompleted);
      }

      #endregion

      #region Properties

      /// 
      /// Whether or not to allow the user to cancel the operation
      /// 
      public bool CanCancel
      {
         set
         {
            mWorkerSupportsCancellation = value;
         }
      }

      #endregion

      #region Operation Start/Stop Details

      /// 
      /// Start the operation with the given parameters
      /// 
      /// The parameters for the worker
      public void StartOperation()
      {
         // Run the worker
         mOperationWorker.RunWorkerAsync(mOperationParameters);
      }

      /// 
      /// Stop the operation
      /// 
      public void StopOperation()
      {
         // Signal the cancel first, then call cancel to stop the test
         if (IsRunning())
         {
            // Sets the backgroundworker CancelPending to true, so we can break
            // in the sub classes operation
            mOperationWorker.CancelAsync();
            // This allows us to trigger an event or "Set" if WaitOne'ing
            Cancel();
            // Wait for it to actually stop before returning
            while (IsRunning())
            {
               Application.DoEvents();
            }
         }
      }

      /// 
      /// Whether or not the operation is currently running
      /// 
      /// 
      public bool IsRunning()
      {
         return mOperationWorker.IsBusy;
      }

      #endregion

      #region BackgroundWorker Events

      /// 
      /// Fires when the operation has completed
      /// 
      /// 
      /// 
      private void OperationWorkerRunWorkerCompleted(object sender, RunWorkerCompletedEventArgs e)
      {
         // Allow the sub class to clean up anything that might need to be updated
         Clean();

         // Notify whoever is register that the operation is complete
         if (OperationCompleted != null)
         {
            OperationCompleted(e);
         }
      }

      ///  
      /// Fires when the progress needs to be updated for a given test (we might not care)
      /// 
      /// 
      /// 
      private void OperationWorkerProgressChanged(object sender, ProgressChangedEventArgs e)
      {
         // Notify whoever is register of what the current percentage is
         if (OperationProgressChanged != null)
         {
            OperationProgressChanged(e);
         }
      }

      /// 
      /// Fires when we start the operation (this does the work)
      /// 
      /// 
      /// 
      private void OperationWorkerDoWork(object sender, DoWorkEventArgs e)
      {
         // Run the operation
         Run(sender, e);
      }

      #endregion

      #region Abstract methods

      /// 
      /// Abstract, implemented in the sub class to do the work
      /// 
      /// 
      /// 
      protected abstract void Run(object sender, DoWorkEventArgs e);

      /// 
      /// Called at the end of the test to clean up anything (ex: Disconnected events, etc)
      /// 
      protected abstract void Clean();

      /// 
      /// If we are waiting on something in the operation, this will allow us to
      /// stop waiting (ex: WaitOne).
      /// 
      protected abstract void Cancel();

      #endregion
   }

SwDevMan81
A: 
SwDevMan81
A: 

@SwDevMan81

Could you repost your example test case again please? A bit seems to be cut off :(

Yeah sorry about that. Darn less than's haha
SwDevMan81
A: 

That code was really good. I'm using here and it's working fine ... but the ReportProgress is giving me an error "Cross-thread operation not valid: Control 'DescriptionLabel' accessed from a thread other than the thread it was created on."

How can I fix it?

Thanks.

Fabio Caldas
It depends on where you are accessing the DescriptionLabel control. Can you post your code?
Brownman98
A: 

I'm currently investigation Threadmare over at http://sklobovsky.nstemp.com/community/threadmare/threadmare.htm for a C# project. It looks very, very useful. It's in Delphi, but the principles apply to any language that can handle events.

AndrewJacksonZA