views:

150

answers:

4

I have a program that uses threads to perform time-consuming processes sequentially. I want to be able to monitor the progress of each thread similar to the way that the BackgroundWorker.ReportProgress/ProgressChanged model does. I can't use ThreadPool or BackgroundWorker due to other constraints I'm under. What is the best way to allow/expose this functionality. Overload the Thread class and add a property/event? Another more-elegant solution?

+4  A: 

Overload the Thread class and add a property/event?

If by "overload" you actually mean inherit then no. The Thread is sealed so it cannot be inherited which means you will not be able to add any properties or events to it.

Another more-elegant solution?

Create a class that encapsulates the logic that will be executed by the thread. Add a property or event (or both) which can be used to obtain progress information from it.

public class Worker
{
  private Thread m_Thread = new Thread(Run);

  public event EventHandle<ProgressEventArgs> Progress;

  public void Start()
  {
    m_Thread.Start();
  }

  private void Run()
  {
    while (true)
    {
      // Do some work.

      OnProgress(new ProgressEventArgs(...));

      // Do some work.
    }
  }

  private void OnProgress(ProgressEventArgs args)
  {
    // Get a copy of the multicast delegate so that we can do the
    // null check and invocation safely. This works because delegates are
    // immutable. Remember to create a memory barrier so that a fresh read
    // of the delegate occurs everytime.
    Thread.MemoryBarrier(); 
    var local = Progress;
    Thread.MemoryBarrier();
    if (local != null)
    {
      local(this, args);
    }
  }
}

Update:

Let me be a little more clear on why a memory barrier is necessary in this situation. The barrier prevents the read from being moved before other instructions. The most likely optimization is not from the CPU, but from the JIT compiler "lifting" the read of Progress outside of the while loop. This movement gives the impression of "stale" reads. Here is a semi-realistic demonstration of the problem.

class Program
{
    static event EventHandler Progress;

    static void Main(string[] args)
    {
        var thread = new Thread(
            () =>
            {
                var local = GetEvent();
                while (local == null)
                {
                    local = GetEvent();
                }
            });
        thread.Start();
        Thread.Sleep(1000);
        Progress += (s, a) => { Console.WriteLine("Progress"); };
        thread.Join();
        Console.WriteLine("Stopped");
        Console.ReadLine();
    }

    static EventHandler GetEvent()
    {
        //Thread.MemoryBarrier();
        var local = Progress;
        return local;
    }
}

It is imperative that a Release build is ran without the vshost process. Either one will disable the optimization that manifest the bug (I believe this is not reproducable in framework version 1.0 and 1.1 as well due to their more primitive optimizations). The bug is that "Stopped" is never displayed even though it clearly should be. Now, uncomment the call to Thread.MemoryBarrier and notice the change in behavior. Also keep in mind that even the most subtle changes to the structure of this code currently inhibit the compiler's ability to make the optimization in question. One such change would be to actually invoke the delegate. In other words you cannot currently reproduce the stale read problem using the null check followed by an invocation pattern, but there is nothing in the CLI specification (that I am aware of anyway) that prohibits a future hypothetical JIT compiler from reapplying that "lifting" optimization.

Brian Gideon
@Brian: you might wish to consider moving the barrier after `var local = ...` http://stackoverflow.com/questions/1773680/thread-volatileread-implementation
andras
@andras: Nah, that would *really* confuse people. It wouldn't matter at all in this case where it is placed. But, the intent here is that the `Progress` read is fresh *always* (not next time, but **now**). I could care less whether the `this` and `args` reads are lifted outside the `if` statement and above the `Progress` read. By the way, I could have used a `lock`, but then a naive programmer might remove it later because it *appeared* pointless. This is one of the few scenarios where I actually recommend `Thread.MemoryBarrier` because it makes the intention plainly obvious.
Brian Gideon
andras
@andras: Yeah, memory barriers are definitely one the most confusing topics there is (aside from the US income tax system). I have actually read all of those links you posted before. I even posted a comment to Eric's blog asking for a clarification on this point recently.
Brian Gideon
@andras: Keep in mind that there are two memory models in play here: The one dictated by the hardware (CPU architecture) and the one dictated by the software (CLR). The model you have to program against is the weaker of the two. In this case its almost always the CLR since, like you said, x86/x64 actually have fairly strong models. That's likely the reason why you won't see the memory fences emitted in compiled code for *those* CPUs. But that doesn't stop the CLR from doing its own reordering.
Brian Gideon
@andras: So is the barrier required or not? Yes, but only on very technical and theorectical grounds. All versions of the CLR through 4.0 will *likely* see "fresh* reads of `Progress` on each and every iteration of the loop even without the explicit barrier. But, there is no guarentee.
Brian Gideon
@Brian: in your update, if you examine the places where a membar is needed to prevent reordering on either the JIT or assembly level, you find two of them: one in `Main()`, after `var local = GetEvent();` and one in the `while` loop, after `local = GetEvent();`. This can be expressed in `GetEvent()` by putting a membar between the assignment and the return statements.
andras
@Brian: which is - not accidentally - is just what a `VolatileRead()` would be.(if there was an overload for events....)
andras
@Brian: a more to the point example for your first code: if you leave the membar where you have put it, there is a theoretical optimization that might take place, namely, `local` can be optimized away. This would translate to `if (Progress != null) Progress()` which is clearly not what is intended. Just to add another great Joe Duffy post: http://www.bluebytesoftware.com/blog/2008/06/13/VolatileReadsAndWritesAndTimeliness.aspx
andras
@Brian: :) after digging through my history: http://code.logos.com/blog/2008/11/events_and_threads_part_4.html (purely theoretical, of course :) What I wanted to suggest - but likely failed - is that the membar is in the wrong place, so either leave it out entirely, or if you want to be absolutely correct on theoretical grounds, do exactly what `volatileread` does and put it in the correct place.
andras
@Brian: I've found yet another example for the correct placing of the membar on loads (just like in your first example, between assignment and check): "Making it work with explicit memory barriers" part in http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html (some great authors here as well. :)
andras
@Brian Gideon - This is exactly the functionality I'm going to employ for my fix. Thanks so much for the time you put into this answer.
Joel B
@Brian: I am not advocating avoiding the membar - you always better be safe than sorry. I was trying to say that you have put it in the wrong place. Coincidentally, it works in the first example - but the first works even without it. Also coincidentally, it works in the second example as well, since the JIT does not do a theoretical optimization that it could have done (e.g. because it would introduce an extra read which is a no-no in the unofficial MS .NET 2.0+ memory model). Having it where it is right now is not the best place for it.
andras
@andras: I think your comment about `local` getting optimized away is the key here. And I see your point. That's definitely not something I was thinking about. I was focused more on the read getting "lifted" outside of the loop. It seems there *should* be a barrier between the read and the check. But, if you simply move it then you now allow the initial read of `Progress` to be "lifted" right? I am going to go ahead and modify my example to include a full fence on both sides of the read. That will fix everything and it still remains more self documenting than an isolated `lock` statement.
Brian Gideon
@Brian: thanks for still bearing with me. :) I appreciate. If you move the barrier then nothing strange will happen. Yes, the read can "float" upwards, but it cannot float down and pass the barrier. As for floating up: it does not really matter. Suppose that the load gets into the buffer and gets served 70 cycles earlier. So what you see there is a "70 cycles old" data. Now suppose that you put a barrier before the read. The barrier waits for the store and load buffers to drain. It takes 100 cycles. By the time you get the supposedly fresh data, 100 cycles passed. Is it still fresh? :)
andras
@Brian: I guess perhaps it is better to forget about barriers as a way to provide "fresh" data - it is imho a better way to think of them as their name suggests, barriers or fences - and put them where you do not want reads and writes to pass either up or down by either the cpu or the compiler. (I guess Joe Duffy was right when he said that even the MSDN description is misleading in a way.)
andras
@Brian: Moving the fence a line down, right after the assignment to a local variable is both sufficient and correct for any of your examples. ( I'd like to correct that "eliminitang reading onto a local var" optimization is purely theoretical. It has been a legal optimization on the x64 JIT: http://blogs.msdn.com/b/grantri/archive/2004/09/07/226355.aspx )
andras
@andras: You are absolutely right. It won't matter if the first `Thread.MemoryBarrier` (the one above the read) is removed because only the initial read *might* be stale, but once the loop swings back around the second time a "refresh" should have occurred. I am totally with you on that.
Brian Gideon
@andras: By the way, you should chime in on the comments to Eric's blog about the thread-safe event invocation pattern. I posted my concern about this a couple of weeks ago and no one has commented since.
Brian Gideon
@Brian: I will chime in, thanks for raising this. :) I guess the example I brought up for the "freshness" issue was not the best, maybe this one is better (but please feel free to raise any objections): consider that the thread is preempted just after reading the value to the local. By the time you examine it in the condition, it is a "stale" value, as you put it. Please note that values are "stale" - it is certainly the wrong word here - anyhow and you cannot do much against this. Memory barriers will not help this. Also, it does not matter, anyway.
andras
A: 

I tried this some time ago and it worked for me.

  1. Create a List-like class with locks.
  2. Have your threads add data to an instance of the class you created.
  3. Place a timer in your Form or wherever you want to record the log/progress.
  4. Write code in the Timer.Tick event to read the messages the threads output.
Alex Essilfie
+1  A: 

You might also want to check out the Event-based Asynchronous Pattern.

YWE
+1 - I'll use Brian's solution for the quick fix, and eventually move to more of an Even-Based Async Pattern for my final product. Thanks for the link!
Joel B
A: 

Provide each thread with a callback that returns a status object. You can use the thread's ManagedThreadId to keep track of separate threads, such as using it as a key to a Dictionary<int, object>. You can invoke the callback from numerous places in the thread's processing loop or call it from a timer fired from within the thread.

You can also use the return argument on a callback to signal the thread to pause or halt.

I've used callbacks with great success.

ebpower