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.

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.

My two cents: there's no real need to force early handle creation if the logging framework simply maintains a buffer of undisplayed log entries while the handle has not been created. It could be implemented as a Queue, or many other things. Messing with the order of handle creation in .NET makes me squeamish.

I think the only danger is decreased performance. Handle creation is deferred in winforms to speed things up. However, since it sound like this is a one-time operation, it doesn't sound costly, so I think your approach is fine.

You can always check the IsHandleCreated property of your form to see if the handle has been built yet; however, there are some caveats. I've been in a similar spot to yours, where winforms controls are being created/destroyed dynamically with lots of multithreading going on. The pattern we wound up using was quite a bit like this:

private void SomeEventHandler(object sender, EventArgs e) // called from a bg thread
{
    MethodInvoker ivk = delegate
    {
        if(this.IsDisposed)
            return; // bail out!  Run away!

        // maybe look for queued stuff if it exists?

        // the code to run on the UI thread
    };

    if(this.IsDisposed)
        return; // run away!  killer rabbits with pointy teeth!

    if(!this.IsHandleCreated) // handle not built yet, do something in the meantime
        DoSomethingToQueueTheCall(ivk);
    else
        this.BeginInvoke(ivk);
}

The big lesson here is to expect a kaboom if you attempt to interact with your form after it has been disposed. Don't rely on InvokeRequired, since it will return false on any thread if the control's handle hasn't been created yet. Also don't rely solely on IsHandleCreated since that will return false after the control has been disposed.

Basically, you have three flags whose state will tell you what you need to know about the control's initialization state and whether or not you're on a BG thread relative to the control.

The control can be in one of three initialization states:

• Uninitialized, no handle created yet
  • InvokeRequired returns false on every thread
  • IsHandleCreated returns false
  • IsDisposed returns false
• Initialized, ready, active
  • InvokeRequired does what the docs say
  • IsHandleCreated returns true
  • IsDisposed returns false
• Disposed
  • InvokeRequired returns false on every thread
  • IsHandleCreated returns false
  • IsDisposed returns true

Hope this helps.

Since you do create the window on the calling thread you can end up with deadlocks. If the thread that creates the window has no message pump running your BeginInvoke will add your delegate call to the message queue which will never be emptied, if you do not have an Application.Run() on the same thread which will process the window messages.

It is also very slow to send around window messages for each log message. It is much better to have a producer consumer model where your logging thread adds a message to a Queue<string> which is emptied by another thread. The only time you need to lock is when you enqueue or dequeue a message. The consumer thread can wait for an event with a timeout to start processing the next message when either the event was signaled or the timeout (e.g. 100ms) has elapsed.

A thread safe blocking Queue can be found here.