c# - Asynchronously raised events which use Invoke causing problems with multithreading

Question

Notice: I do not control the UI code. Only the class PeriodicalThing code.

---

Consider this scenario:

The UI thread is happily doing UI stuff, while there is another background thread which periodically raises events. The events are subscribed to by some UI, which means the event handlers generally have to use Invoke.

What I need is a way to perform the clean up operation (cleaning up means stopping the background thread, basically) in safe manner, that guarantees:

1. User-defined code (i.e. event handler code) is not asynchronously aborted in the middle of execution
2. There are no more periodical operations executed or executing after the clean up function has returned
3. No more event handlers will be executed or executing after the clean up function has returned

I came up with something, but there is a deadlock. The deadlock is basically an error in the user-defined code, where the usage of BeginInvoke could have fixed the problem, but a straight out deadlock in case of a non-trivial programming error isn't the way to go.
Also note that BeginInvoke only happens to work in the FormClosing scenario because the invocation list of a Form happens to be cleared after FormClosing; something that seems to be consistent but I haven't found it documented yet.

If there is a solution, it's apparently not obvious, but maybe I'm missing out a trick. I can't believe noone has run into a similar problem before.

class PeriodicalThing
{
    bool abort = false;
    Thread PeriodicalThread;
    ...
    PeriodicalThreadProc()
    {
        while (!this.abort)
        {
            DoStuff();
            OnThingsHappened(new EventArgs(...));
        }
    }

    public event EventHandler<EventArgs> ThingsHappened;
    protected virtual void OnThingsHappaned(EventArgs e)
    {
        // update -- oversight by me - see Henk Holterman's answer
        var handler = this.ThingsHappened;
        if (handler != null)
        {
            handler(this, e);
        }
    }

    public void CleanUp()
    {
        this.abort = true;
        // ui thread will deadlock here
        this.PeriodicalThread.Join();
    }
}

...

// user-defined code; consider this immutable
class Form1 : Form
{
    .ctor()
    {
        ...
        this.PeriodicalThing.ThingsHappened += this.ThingsHappenedHandler
    }

    private void ThingsHappenedHandler(object sender, EventArgs e)
    {
        if (this.InvokeRequired) // actually always true
        {
            // periodical thread will deadlock here
            this.Invoke(
                new Action<object, EventArgs>(this.ThingsHappenedHandler), sender, e)
                );
            return;
        }
        this.listBox1.Items.Add("things happened");
    }

    private void Form1_FormClosing(object sender, FormClosingEventArgs e)
    {
        this.PeriodicalThing.CleanUp();
    }
}

When the UI thread is shutting down due to an event such as FormClosing, which happened on the UI thread, then it will trigger the clean up. When the background thread issues an Invoke at this time, the Invoke has to block until the UI thread is finished with its current event handler (which triggered the clean up in the first place). Also, the clean up operation needs to wait for the background thread (and thus the current Invoke) to terminate, causing a deadlock.

---

The optimal solution would be to interrupt the UI thread at Thread.Join() and let all the waiting Invokes execute, and then going back to Thread.Join(). But that seems impossible to me with C#. Maybe someone has a crazy idea how I could use some helper threads to move the clean up method away from the UI thread, but I don't know how I would do that.

Answer 1

The problem is here

public void CleanUp()
{
    this.abort = true;
    // ui thread will deadlock here
    this.PeriodicalThread.Join();  // just delete this
}

The Join() will block (!) the calling thread. And that in turn blocks all Invoke actions.

The other part of this coin is

{
  if (this.InvokeRequired) // actually always true
    {
        // periodical thread will deadlock here
    //    this.Invoke(
        this.BeginInvoke(            // doesn't wait so it doesn't block
            new Action<object, EventArgs>(this.ThingsHappenedHandler), sender, e)
            );
        return;
    }
    this.listBox1.Items.Add("things happened");
 }

BeginInvoke is an improvement over Invoke anyway, as long as you only return void and do not overload the MessageLoop.

But do get rid of the Join(). It has no use.

Edit, since some parts are not under your control:

The following is indeed the answer and it's possible:

to interrupt the UI thread at Thread.Join() and let all the waiting Invokes execute, and then going back to Thread.Join()

public void CleanUp()
{
    this.abort = true;
 
    while (! this.PeriodicalThread.Join(20)) 
    { 
       Application.DoEvents(); 
    }
}

This won't deadlock but there are issues with using Application.DoEvents(). You'll have to check what happens in all other events (FormClosing) and that's not under your control too...
It'll probably work but some rigorous testing is called for.

---

Since you are mixing threads and events, use this pattern:

protected virtual void OnThingsHappaned(EventArgs e)
{
    var handler = ThingsHappened;

    if (handler  != null)
    {
        handler (this, e);
    }
}