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

Question

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.

Crankcase

What if logger thread just write into Queue, and window picks from it messages in sequence as soon as it is able to do that? You will avoid comunication problem: someone put's message in "basket", someone who wants to show takes it from the basket when he can... – Carpentaria

Why don't you use some existing solution, like NLog? – Lundeen

@Lundeen - Again, doesn't address my question. There are very specific reasons why I don't use NLog or Log4net in this app. Those reasons are irrelevant to the question. – Crankcase

@mrjoltcola, You have to implement two MPSC queues with strong FIFO, which is not a very easy task for a person without specific knowledge and skills. Single post on Q'n'A forum is just not enough to explain all you need to know. So, it is likely that is will be just easier task to implement an extension to existing logging framework and quality of solution will be higher. However, just as I said before, complete solution look like two MPSC queues. One custom and one windows messages based. – Lundeen

@adontz: You are still missing the question. There are umpteen examples on Google about this very pattern, and most of them implement queues, messages or timers to fix this. What I'm saying is "My approach seems to simple to be correct." Forget that I mentioned logging; this could be any race-condition where a thread waits on a foreground window. – Crankcase

@mrjoltcola, using BeginInvoke is using windows message based queue. Why do you think it is "too simple"? You eliminated need of first queue I mentioned by forcing window handle creation. – Lundeen

Answer 1

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.

Archie

Answer 2

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.

Halsted

Egh, this is similar to Tigran's comment; I should have read above. – Halsted

The only other comment I have is that you should stress-test the thing by issuing many log requests from separate threads. If it turns out that your logging code adds to the log control in a non-atomic fashion, the text could become mixed up or corrupt, in which case you should add a mutex. – Halsted

Fine answer, but doesn't address the actual question. So, to rephrase, is there any danger to forcing the Handle to create in a constructor? – Crankcase

As far as I understand it, the only danger is decreased performance. Handle creation is deferred in winforms to speed things up. – Halsted

In this case, the window will always be created, so the performance isn't a problem. Anything else you can think of? If you think of anything, if you don't mind editing your answer above, I'll reverse the vote. I'm just trying to keep the focus on the Handle issue, not alternatives. Thanks! – Crankcase

Answer 3

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.

Backspace

Answer 4

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.

Never

Not true. The calling thread uses ShowDialog() which handles the message queue just fine. Also, this is a single user Winforms app. Performance of updating a dialog is not a concern, I'm just updating a progress bar and a grid, one of the oldest techniques in the book. – Crankcase

You did ask for potential issues. If more than one thread is involved which can issue log calls you will run into this one if you are not careful. It all depends how you let access the other threads your logger form. If it is a static object it can be accessed by any other thread and initialize your form on the wrong thread. – Never

Reading this old thread again, I feel your answer had merit, and did not deserve my downvote. I was so focused on the particular question at hand, I was hasty with my vote. Please edit your answer so I can reverse the downvote. I appreciate that you took the time to point out potential performance issues. – Crankcase

Fixed some typos. You cannot change the downvote after some time? Anyway I hope the changes are sufficient. – Never

Yes, after a time, you cant change vote unless the post is edited. Done. – Crankcase