Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Need second (and third) opinions on my fix for this Winforms race condition

There are a hundred examples in blogs, etc. on how to implement a background worker that logs or gives status to a foreground GUI element. Most of them include an approach to handle the race condition that exists between spawning the worker thread, and creating the foreground dialog with ShowDialog(). However, it occured to me that a simple approach is to force the creation of the handle in the form constructor, so that the thread won't be able to trigger an Invoke/BeginInvoke call on the form prior to its handle creation.

Consider a simple example of a Logger class that uses a background worker thread to log to the foreground.

Assume, also, that we don't want NLog or some other heavy framework to do something so simple and lightweight.

My logger window is opened with ShowDialog() by the foreground thread, but only after the background "worker" thread is started. The worker thread calls logger.Log() which itself uses logForm.BeginInvoke() to update the log control correctly on the foreground thread.

  public override void Log(string s)
  {
     form.BeginInvoke(logDelegate, s);
  }

Where logDelegate is just a simple wrapper around "form.Log()" or some other code that may update a progress bar.

The problem lies in the race condition that exists; when the background worker thread starts logging before the foreground ShowDialog() is called the form's Handle hasn't yet been created so the BeginInvoke() call fails.

I'm familiar with the various approaches, including using a Form OnLoad event and a timer to create the worker task suspended until the OnLoad event generates a timer message that starts the task once the form is shown, or, as mentioned, using a queue for the messages. However, I think that simply forcing the dialog's handle to create early (in the constructor) ensures there is no race condition, assuming the thread is spawned off by the same thread that creates the dialog.

http://msdn.microsoft.com/en-us/library/system.windows.forms.control.handle(v=vs.71).aspx

MSDN says: "If the handle has not yet been created, referencing this property will force the handle to be created."

So my logger wraps a form, and its constructor does:

   public SimpleProgressDialog() {
       var h = form.Handle; // dereference the handle
   }

The solution seems too simple to be correct. I'm specifically interested in why the seemingly too simple solution is or isn't safe to use.

Any comments? Am I missing something else?

EDIT: I'm NOT asking for alternatives. Not asking how to use NLog or Log4net, etc. if I were, I'd write a page about all of the customer constraints on this app, etc.

By the number of upvotes, there are a lot of other people that would like to know the answer too.

like image 469
codenheim Avatar asked Jul 22 '11 21:07

codenheim


1 Answers

If you are concerned that referencing Control.Handle relies on a side effect in order to create the handle, you can simply call Control.CreateControl() to create it. However, referencing the property has the benefit of not initializing it if it already exists.

As for whether this is safe or not assuming the handle is created, you are correct: as long as you create the handle before spawning the background task on the same thread, you will avoid a race condition.

like image 175
MSN Avatar answered Sep 18 '22 20:09

MSN