views:

2816

answers:

11

I'm still plagued by background threading in a WinForm UI. Why? Here are some of the issues:

  1. Obviously the most important issue, I can not modify a Control unless I'm executing on the same thread that created it.
  2. As you know, Invoke, BeginInvoke, etc are not available until after a Control is created.
  3. Even after RequiresInvoke returns true, BeginInvoke can still throw ObjectDisposed and even if it doesn't throw, it may never execute the code if the control is being destroyed.
  4. Even after RequiresInvoke returns true, Invoke can indefinitely hang waiting for execution by a control that was disposed at the same time as the call to Invoke.

I'm looking for an elegant solution to this problem, but before I get into specifics of what I'm looking for I thought I would clarify the problem. This is to take the generic problem and put a more concrete example behind it. For this example let's say we are transferring larger amounts of data over the internet. The user interface must be able to show a progress dialog for the transfer already in-progress. The progress dialog should update constantly and quickly (updates 5 to 20 times per second). The user can dismiss the progress dialog at any time and recall it again if desired. And further, lets pretend for arguments sake that if the dialog is visible, it must process every progress event. The user can click Cancel on the progress dialog and via modifying the event args, cancel the operation.

Now I need a solution that will fit in the following box of constraints:

  1. Allow a worker thread to call a method on a Control/Form and block/wait until execution is complete.
  2. Allow the dialog itself to call this same method at initialization or the like (and thus not use invoke).
  3. Place no burden of implementation on the handling method or the calling event, the solution should only change the event subscription itself.
  4. Appropriately handle blocking invokes to a dialog that might be in the process of disposing. Unfortunately this is not as easy as checking for IsDisposed.
  5. Must be able to be used with any event type (assume a delegate of type EventHandler)
  6. Must not translate exceptions to TargetInvocationException.
  7. The solution must work with .Net 2.0 and higher

So, can this be solved given the constraints above? I've searched and dug through countless blogs and discussions and alas I'm still empty handed.

Update: I do realize that this question has no easy answer. I've only been on this site for a couple of days and I've seen some people with a lot of experience answering questions. I'm hoping that one of these individuals has solved this sufficiently enough for me to not spend the week or so it will take to build a reasonable solution.

Update #2: Ok, I'm going to try and describe the problem in a little more detail and see what (if anything) shakes out. The following properties that allow us to determine it's state have a couple of things raise concerns...

  1. Control.InvokeRequired = Documented to return false if running on current thread or if IsHandleCreated returns false for all parents. I'm troubled by the InvokeRequired implementation having the potential to either throw ObjectDisposedException or potentially even re-create the object's handle. And since InvokeRequired can return true when we are not able to invoke (Dispose in progress) and it can return false even though we might need to use invoke (Create in progress) this simply can't be trusted in all cases. The only case I can see where we can trust InvokeRequired returning false is when IsHandleCreated returns true both before and after the call (BTW the MSDN docs for InvokeRequired do mention checking for IsHandleCreated).

  2. Control.IsHandleCreated = Returns true if a handle has been assigned to the control; otherwise, false. Though IsHandleCreated is a safe call it may breakdown if the control is in the process of recreating it's handle. This potential problem appears to be solveable by performing a lock(control) while accessing the IsHandleCreated and InvokeRequired.

  3. Control.Disposing = Returns true if the control is in the process of disposing.

  4. Control.IsDisposed = Returns true if the control has been disposed. I'm considering subscribing to the Disposed event and checking the IsDisposed property to determin if BeginInvoke will ever complete. The big problem here is the lack of a syncronization lock durring the Disposing -> Disposed transition. It's possible that if you subscribe to the Disposed event and after that verify that Disposing == false && IsDisposed == false you still may never see the Disposed event fire. This is due to the fact that the implementation of Dispose sets Disposing = false, and then sets Disposed = true. This provides you an oppertunity (however small) to read both Disposing and IsDisposed as false on a disposed control.

... my head hurts :( Hopefully the information above will shed a little more light on the issues for anyone having these troubles. I appreciate your spare thought cycles on this.

Closing in on the trouble... The following is the later half of the Control.DestroyHandle() method:

if (!this.RecreatingHandle && (this.threadCallbackList != null))
{
    lock (this.threadCallbackList)
    {
        Exception exception = new ObjectDisposedException(base.GetType().Name);
        while (this.threadCallbackList.Count > 0)
        {
            ThreadMethodEntry entry = (ThreadMethodEntry) this.threadCallbackList.Dequeue();
            entry.exception = exception;
            entry.Complete();
        }
    }
}
if ((0x40 & ((int) ((long) UnsafeNativeMethods.GetWindowLong(new HandleRef(this.window, this.InternalHandle), -20)))) != 0)
{
    UnsafeNativeMethods.DefMDIChildProc(this.InternalHandle, 0x10, IntPtr.Zero, IntPtr.Zero);
}
else
{
    this.window.DestroyHandle();
}

You'll notice the ObjectDisposedException being dispatched to all waiting cross-thread invocations. Shortly following this is the call to this.window.DestroyHandle() which in turn destroys the window and set's it's handle reference to IntPtr.Zero thereby preventing further calls into the BeginInvoke method (or more precisely MarshaledInvoke which handle both BeginInvoke and Invoke). The problem here is that after the lock releases on threadCallbackList a new entry can be inserted before the Control's thread zeros the window handle. This appears to be the case I'm seeing, though infrequently, often enough to stop a release.

Update #4:

Sorry to keep dragging this on; however, I thought it worth documenting here. I've managed to solve most of the problems above and I'm narrowing in on a solution that works. I've hit one more issue I was concerned about, but until now, have not seen 'in-the-wild'.

This issue has to do with the genius that wrote Control.Handle property:

 public IntPtr get_Handle()
 {
  if ((checkForIllegalCrossThreadCalls && !inCrossThreadSafeCall) && this.InvokeRequired)
  {
   throw new InvalidOperationException(SR.GetString("IllegalCrossThreadCall", new object[] { this.Name }));
  }
  if (!this.IsHandleCreated)
  {
   this.CreateHandle();
  }
  return this.HandleInternal;
 }

This by itself is not so bad (regardless of my opinions on get { } modifications); however, when combined with the InvokeRequired property or the Invoke/BeginInvoke method it is bad. Here is the basic flow the Invoke:

if( !this.IsHandleCreated )
    throw;
... do more stuff
PostMessage( this.Handle, ... );

The issue here is that from another thread I can successfully pass through the first if statement, after which the handle is destroyed by the control's thread, thus causing the get of the Handle property to re-create the window handle on my thread. This then can cause an exception to be raised on the original control's thread. This one really has me stumped as there is no way to guard against this. Had they only use the InternalHandle property and tested for result of IntPtr.Zero this would not be an issue.

+12  A: 

Your scenario, as described, neatly fits BackgroundWorker - why not just use that? Your requirements for a solution are way too generic, and rather unreasonable - I doubt there is any solution that would satisfy them all.

Pavel Minaev
I agree with your sentiment, this is a difficult problem. One I'm sure a lot of people have faced. The reason I pose this question is that I do not believe there is a solution; however, I'm hoping someone can prove me wrong.
csharptest.net
Pavel, thank you for directing me at BackgroundWorker, I was unaware of it's existence. It does beautifully fit the scenario I described and I will definitely find use for it.
csharptest.net
It's not elegant, but I think you'll appreciate the solution I posted can work without suffering from the threading issues that Invoke, BeginInvoke, and BackgroundWorker all produce. Provided that you can be certain that your form does not close before completion, and that the form does not re-create it's handle, a BackgroundWorker will still do the job nicely.
csharptest.net
Before searching the blogs and news groups and other 'junk'...search the Object Browser. I heard there over 70,000 objects in the .NET Framework to choose from. If you can't find what you are looking for, you will find items to help you better search other 'junk'.
AMissico
A: 

If you don't like the BackgroundWoker (as described by @Pavel) you might want to look at this library http://www.wintellect.com/PowerThreading.aspx.

Kane
+2  A: 

I'm not going to write an exhaustive solution for you that meets all of your requirements, but I will offer perspective. Overall, though, I think you're shooting for the moon with those requirements.

The Invoke/BeginInvoke architecture simply executes a supplied delegate on the control's UI thread by sending it a Windows message and the message loop itself executes the delegate. The specific workings of this are irrelevant, but the point is that there is no particular reason that you have to use this architecture for thread sync with the UI thread. All you need is some other loop running, such as in a Forms.Timer or something like that, that monitors a Queue for delegates to execute and does so. It would be fairly simple to implement your own, though I don't know what specifically it would get for you that Invoke and BeginInvoke don't provide.

Adam Robinson
I COMPLETELY agree, this is/has been my preference for most things (a simple producer/consumer queue). My problem with this solution is two-fold. A) It requires a fair amount of impact to the client code (code running in the winform); and B) It does not easily allow blocking/bi-directional events. Of course you can do it, create a wait handle, wait for the consumer to process the message, then continue; however, this puts you right back in the same boat as you started.... dealing with the potential of the UI being disposed by the user.
csharptest.net
At some level you're going to be left with that, good or bad. As others have suggested it's not necessary that you actually dispose the progress dialog when the user closes it.
Adam Robinson
A: 

If I understand this, why do you ever need to dispose the progress dialog while the application is running? Why not just show and hide it on the users request? This sounds like it will make your problem at least a little bit simpler.

alexD
Agreed, in this scenario one could argue to simply hide the dialog until the process is complete; however, I'm using this as a generic means of discussing a broader problem I am facing in WinForms development in general. Your spot-on for this specific problem but I can't apply the principal to all situations.
csharptest.net
+1  A: 

This is not really answer to the second part of the question, but I will include it just for the reference:

private delegate object SafeInvokeCallback(Control control, Delegate method, params object[] parameters);
public static object SafeInvoke(this Control control, Delegate method, params object[] parameters)
{
    if (control == null)
     throw new ArgumentNullException("control");
    if (control.InvokeRequired)
    {
     IAsyncResult result = null;
     try { result = control.BeginInvoke(new SafeInvokeCallback(SafeInvoke), control, method, parameters); }
     catch (InvalidOperationException) { /* This control has not been created or was already (more likely) closed. */ }
     if (result != null)
      return control.EndInvoke(result);
    }
    else
    {
     if (!control.IsDisposed)
      return method.DynamicInvoke(parameters);
    }
    return null;
}

This code should avoid the most common pitfalls with Invoke/BeginInvoke and it's easy to use . Just turn

if (control.InvokeRequired)
    control.Invoke(...)
else
    ...

into

control.SafeInvoke(...)

Similar construct is possible for BeginInvoke.

Filip Navara
+2  A: 

Ok, days later I've finished creating a solution. It solves all of the listed constraints and objectives in the initial post. The usage is simple and straight-forward:

myWorker.SomeEvent += new EventHandlerForControl<EventArgs>(this, myWorker_SomeEvent).EventHandler;

When the worker thread calls this event it will handle the required invocation to the control thread. It ensures that it will not hang indefinitely and will consistently throw an ObjectDisposedException if it is unable to execute on the control thread. I've created other derivations of the class, one to ignore the error, and another to directly call the delegate if the control is not available. Appears to work well and fully passes the several tests that reproduce the issues above. There is only one issue with the solution I can't prevent without violating constraint #3 above. This issue is the last (Update #4) in the issue description, the threading problems in get Handle. This can cause unexpected behavior on the original control thread, and I've regularly seen InvalidOperationException() thrown while calling Dispose() since the handle in the process of being created on my thread. To allow for dealing with this I ensure a lock around accessing functions that will use the Control.Handle property. This allows a form to overload the DestroyHandle method and lock prior to calling the base implementation. If this is done, this class should be entirely thread-safe (To the best of my knowledge).

public class Form : System.Windows.Forms.Form
{
 protected override void DestroyHandle()
 {
  lock (this) base.DestroyHandle();
 }
}

You may notice the core aspect of solving the dead-lock became a polling loop. Originally I successfully solved the test cases by handling the control's event for Disposed and HandleDestroyed and using multiple wait handles. After a more careful review, I found the subscription/unsubscription from these events is not thread-safe. Thus I chose to poll the IsHandleCreated instead so as not to create unneeded contention on the thread's events and thereby avoid the possibility of still producing a dead-lock state.

Anyway, here is the solution I came up with:

/// <summary>
/// Provies a wrapper type around event handlers for a control that are safe to be
/// used from events on another thread.  If the control is not valid at the time the
/// delegate is called an exception of type ObjectDisposedExcpetion will be raised.
/// </summary>
[System.Diagnostics.DebuggerNonUserCode]
public class EventHandlerForControl<TEventArgs> where TEventArgs : EventArgs
{
 /// <summary> The control who's thread we will use for the invoke </summary>
 protected readonly Control _control;
 /// <summary> The delegate to invoke on the control </summary>
 protected readonly EventHandler<TEventArgs> _delegate;

 /// <summary>
 /// Constructs an EventHandler for the specified method on the given control instance.
 /// </summary>
 public EventHandlerForControl(Control control, EventHandler<TEventArgs> handler)
 {
  if (control == null) throw new ArgumentNullException("control");
  _control = control.TopLevelControl;
  if (handler == null) throw new ArgumentNullException("handler");
  _delegate = handler;
 }

 /// <summary>
 /// Constructs an EventHandler for the specified delegate converting it to the expected
 /// EventHandler&lt;TEventArgs> delegate type.
 /// </summary>
 public EventHandlerForControl(Control control, Delegate handler)
 {
  if (control == null) throw new ArgumentNullException("control");
  _control = control.TopLevelControl;
  if (handler == null) throw new ArgumentNullException("handler");

  //_delegate = handler.Convert<EventHandler<TEventArgs>>();
  _delegate = handler as EventHandler<TEventArgs>;
  if (_delegate == null)
  {
   foreach (Delegate d in handler.GetInvocationList())
   {
    _delegate = (EventHandler<TEventArgs>) Delegate.Combine(_delegate,
     Delegate.CreateDelegate(typeof(EventHandler<TEventArgs>), d.Target, d.Method, true)
    );
   }
  }
  if (_delegate == null) throw new ArgumentNullException("_delegate");
 }


 /// <summary>
 /// Used to handle the condition that a control's handle is not currently available.  This
 /// can either be before construction or after being disposed.
 /// </summary>
 protected virtual void OnControlDisposed(object sender, TEventArgs args)
 {
  throw new ObjectDisposedException(_control.GetType().Name);
 }

 /// <summary>
 /// This object will allow an implicit cast to the EventHandler&lt;T> type for easier use.
 /// </summary>
 public static implicit operator EventHandler<TEventArgs>(EventHandlerForControl<TEventArgs> instance)
 { return instance.EventHandler; }

 /// <summary>
 /// Handles the 'magic' of safely invoking the delegate on the control without producing
 /// a dead-lock.
 /// </summary>
 public void EventHandler(object sender, TEventArgs args)
 {
  bool requiresInvoke = false, hasHandle = false;
  try
  {
   lock (_control) // locked to avoid conflicts with RecreateHandle and DestroyHandle
   {
    if (true == (hasHandle = _control.IsHandleCreated))
    {
     requiresInvoke = _control.InvokeRequired;
     // must remain true for InvokeRequired to be dependable
     hasHandle &= _control.IsHandleCreated;
    }
   }
  }
  catch (ObjectDisposedException)
  {
   requiresInvoke = hasHandle = false;
  }

  if (!requiresInvoke && hasHandle) // control is from the current thread
  {
   _delegate(sender, args);
   return;
  }
  else if (hasHandle) // control invoke *might* work
  {
   MethodInvokerImpl invocation = new MethodInvokerImpl(_delegate, sender, args);
   IAsyncResult result = null;
   try
   {
    lock (_control)// locked to avoid conflicts with RecreateHandle and DestroyHandle
     result = _control.BeginInvoke(invocation.Invoker);
   }
   catch (InvalidOperationException)
   { }

   try
   {
    if (result != null)
    {
     WaitHandle handle = result.AsyncWaitHandle;
     TimeSpan interval = TimeSpan.FromSeconds(1);
     bool complete = false;

     while (!complete && (invocation.MethodRunning || _control.IsHandleCreated))
     {
      if (invocation.MethodRunning)
       complete = handle.WaitOne();//no need to continue polling once running
      else
       complete = handle.WaitOne(interval);
     }

     if (complete)
     {
      _control.EndInvoke(result);
      return;
     }
    }
   }
   catch (ObjectDisposedException ode)
   {
    if (ode.ObjectName != _control.GetType().Name)
     throw;// *likely* from some other source...
   }
  }

  OnControlDisposed(sender, args);
 }

 /// <summary>
 /// The class is used to take advantage of a special-case in the Control.InvokeMarshaledCallbackDo()
 /// implementation that allows us to preserve the exception types that are thrown rather than doing
 /// a delegate.DynamicInvoke();
 /// </summary>
 [System.Diagnostics.DebuggerNonUserCode]
 private class MethodInvokerImpl
 {
  readonly EventHandler<TEventArgs> _handler;
  readonly object _sender;
  readonly TEventArgs _args;
  private bool _received;

  public MethodInvokerImpl(EventHandler<TEventArgs> handler, object sender, TEventArgs args)
  {
   _received = false;
   _handler = handler;
   _sender = sender;
   _args = args;
  }

  public MethodInvoker Invoker { get { return this.Invoke; } }
  private void Invoke() { _received = true; _handler(_sender, _args); }

  public bool MethodRunning { get { return _received; } }
 }
}

If you see anything wrong here, please let me know.

csharptest.net
Anyone have a better solution? I'll post a bounty for a few days and see what turns up.
csharptest.net
A: 

Why not just hide the dialog when the user dismisses it? That should work fine if you don't show that dialog modally. (use show instead of showdialog). I believe that you can keep your progress dialog on top of your owning window (if you need to) by passing the host to the dialog when you call show.

JMarsch
A: 

Wow, long question. I'll try to organize my answer so you can correct me if I understood something wrong, ok?

1) Unless you have an extremely good reason to call UI methods directly from different threads, don't. You can always go for the producer/consumer model using event handlers:

protected override void OnLoad()
{
    //...
    component.Event += new EventHandler(myHandler);
}

protected override void OnClosing()
{
    //...
    component.Event -= new EventHandler(myHandler);
}

myHandler will be fired every time the component in a different thread needs to perform something in the UI, for example. Also, setting up the event handler in OnLoad and unsubscribing in OnClosing makes sure that the events will only be received/handled by the UI while its handle is created and ready to process the events. You won't even be able to fire events to this dialog if it is in the process of disposing, because you won't be subscribed to the event anymore. If another event is fired while one is still being processed, it will be queued.

You can pass all the info you need in the event arguments: whether you're updating the progress, closing the window, etc.

2) You don't need InvokeRequired if you use the model I suggested above. In this example, you know that the only thing that is firing myHandler will be your component that lives in another thread, for example.

private void myHandler(object sender, EventArgs args)
{
    BeginInvoke(Action(myMethod));
}

So you can always use invoke to make sure you'll be in the right thread.

3) Beware of synchronous calls. If you want to, you can replace use Invoke instead of BeginInvoke. This will block your component until the event has been processed. However, if in the UI you need to communicate to something that is exclusive to the thread your component lives in, you can have deadlock problems. (I don't know if I made myself clear, please let me know). I've had problems with exceptions when using reflection (TargetInvocationException) and BeginInvoke (as they start a different thread, you lose part of the stack trace), but I don't recall having a lot of trouble with Invoke calls, so you should be safe when it comes to exceptions.

Whoa, long answer. If by any chance I missed any of your requirements or misunderstood something you said (english is not my native language, so we're never sure), please let me know.

diogoriba
+1  A: 

I try to organize all such invoke messages to the GUI as fire and forget (handling the exception the GUI can throw due to the race condition on disposing the form).

This way if it never executes no harm is done.

If the GUI needs to respond to the working thread it has a way of effectively reversing the notification. For simple needs BackgroundWorker already handles this.

KeeperOfTheSoul
+3  A: 

I ran into this problem awhile back and came up with solution involving Synchronization Contexts. The solution is to add an extension method to SynchronizationContext which binds a particular delegate to the thread that the SynchronizationContext is bound to. It will generate a new delegate which when invoked will marshal the call to the appropraite thread and then call the original delegate. It makes it nearly impossible for consumers of the delegate to call it in the wrong context.

Blog post on the subject:

JaredPar
A nice solution, with a little work it could event avoid the DynamicInvoke call and thus not generate TargetInvocationException. All in all I like the approach, but I have one question: What is the overhead of generating methods? Do they get unloaded when no longer in use? Additionally, I'm still a little confused why you chose the method generation approach as opposed to a simple class that wraps the delegate.
csharptest.net
A: 

Using System.ComponentModel.ISynchronizeInvoke is nice when creating a System.ComponentModel.Component, such as the BackgroundWorker. The following code snippet is how the FileSystemWater handles events.

    ''' <summary>
    ''' Gets or sets the object used to marshal the event handler calls issued as a result of finding a file in a search.
    ''' </summary>
    <IODescription(SR.FSS_SynchronizingObject), DefaultValue(CType(Nothing, String))> _
    Public Property SynchronizingObject() As System.ComponentModel.ISynchronizeInvoke
        Get
            If (_synchronizingObject Is Nothing) AndAlso (MyBase.DesignMode) Then
                Dim oHost As IDesignerHost = DirectCast(MyBase.GetService(GetType(IDesignerHost)), IDesignerHost)
                If (Not (oHost Is Nothing)) Then
                    Dim oRootComponent As Object = oHost.RootComponent
                    If (Not (oRootComponent Is Nothing)) AndAlso (TypeOf oRootComponent Is ISynchronizeInvoke) Then
                        _synchronizingObject = DirectCast(oRootComponent, ISynchronizeInvoke)
                    End If
                End If
            End If
            Return _synchronizingObject
        End Get
        Set(ByVal Value As System.ComponentModel.ISynchronizeInvoke)
            _synchronizingObject = Value
        End Set
    End Property

    Private _onStartupHandler As EventHandler

    Protected Sub OnStartup(ByVal e As EventArgs)
        If ((Not Me.SynchronizingObject Is Nothing) AndAlso Me.SynchronizingObject.InvokeRequired) Then
            Me.SynchronizingObject.BeginInvoke(_onStartupHandler, New Object() {Me, e})
        Else
            _onStartupHandler.Invoke(Me, e)
        End If
    End Sub
AMissico