C# Threading/Lock confusion

Question

I have the following code:

var items = new List<string> {"1", "2", "3"}; // 200 items
foreach(var item in items) {
  ThreadPool.QueueUserWorkItem((DoWork), item);
}

private void DoWork(object obj)
{
  lock(this)
  {
    using(var sw = File.AppendText(@"C:\somepath.txt")
    {
      sw.WriteLine(obj);
    }
  }
}

Because of the threading, for some reason, I get a random number of the 200 items written out to the file. 60 or 127 or sometimes only 3. If I remove the ThreadPool and just write inside the original foreach loop, all 200 items are written successfully.

Not sure why this occurs?

Thanks for your help.

Wim Coenen · Accepted Answer · 2009-08-15 13:20:04Z

9

The following note from MSDN documentation on ThreadPool says it all:

The threads in the managed thread pool are background threads. That is, their IsBackground properties are true. This means that a ThreadPool thread will not keep an application running after all foreground threads have exited.

Your application simply exits (by reaching end of Main) before your threads finish running.

edited Aug 15, 2009 at 13:20

Wim Coenen

67k14 gold badges162 silver badges253 bronze badges

answered Aug 14, 2009 at 23:59

Pavel Minaev

102k27 gold badges222 silver badges293 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Steven Sudit Over a year ago

One way to fix this is to use Interlocked.Increment to keep track of how many times DoWork is called, setting an auto-reset event when it hits the magic number. The main thread can wait on that event instead of shutting down.

shahkalpesh Over a year ago

Isn't there a way to tell main thread to wait till all the ThreadPool threads are done with the work?

Pavel Minaev Over a year ago

Steven's comment explains how to do that.

Steven Sudit · Accepted Answer · 2009-09-30 15:24:40Z

This is a simple version of what I was alluding to. It uses a single event and does not poll or spin, and it's written so as to be reusable as well as allowing multiple work sets at the same time. The lambda expressions could be factored out, if that's more convenient for debugging.

class Program
{
    static void Main(string[] args)
    {
        var items = new string[] { "1", "2", "3", "300" };
        using (var outfile = File.AppendText("file.txt"))
        {
            using (var ws = new WorkSet<string>(x =>
                    { lock (outfile) outfile.WriteLine(x); }))
                foreach (var item in items)
                    ws.Process(item);
        }
    }

    public class WorkSet<T> : IDisposable
    {
        #region Interface

        public WorkSet(Action<T> action)
        { _action = action; }

        public void Process(T item)
        {
            Interlocked.Increment(ref _workItems);
            ThreadPool.QueueUserWorkItem(o =>
                    { try { _action((T)o); } finally { Done(); } }, item);
        }

        #endregion
        #region Advanced
        public bool Done()
        {
            if (Interlocked.Decrement(ref _workItems) != 0)
                return false;

            _finished.Set();
            return true;
        }

        public ManualResetEvent Finished
        { get { return _finished; } }

        #endregion
        #region IDisposable

        public void Dispose()
        {
            Done();
            _finished.WaitOne();
        }

        #endregion
        #region Fields

        readonly Action<T> _action;
        readonly ManualResetEvent _finished = new ManualResetEvent(false);
        int _workItems = 1;

        #endregion
    }
}

LunaTick · Accepted Answer · 2009-08-16 13:50:48Z

0

How about short and sweet?

    static int wrkThreads = 0;
    static readonly EventWaitHandle exit = new ManualResetEvent(false);
    static readonly object syncLock = new object();

    static void Main( string[] items )
    {
        wrkThreads = items.Length;

        foreach ( var item in items )
            ThreadPool.QueueUserWorkItem(( DoWork ), item);

        exit.WaitOne();
    }

    static void DoWork( object obj )
    {
        lock ( syncLock ) {
            /* Do your file work here */
        }
        if ( Interlocked.Decrement(ref wrkThreads) == 0 )
            exit.Set();
    }

answered Aug 16, 2009 at 13:50

LunaTick

8 Comments

Steven Sudit Over a year ago

Right, this is the algorithm I posted, only stripping out all reusability. It also requires that you know how many work items there'll be, in advance, whereas the solution I offered does not. However, it does refute Spencer's suggestion that this algorithm is itself long.

LunaTick Over a year ago

That it is. Reason I posted it is that considering the question, the OP is not yet big on multi-threading and this short version solves his problem while clearly demonstrating a rudimentary thread synchronization method.

Steven Sudit Over a year ago

I understand your motivation, but I'm not wild about the fact that you use a generic object to lock on instead of the FileStream itself.

LunaTick Over a year ago

I'm not wild that you use locally scoped object to lock on either. Matters little as long as it leads to predictable outcome.

Steven Sudit Over a year ago

It's not local; the FileStream is scoped into the anonymous delegate through a closure. Since the inner using block does not end until all threads do, the FileStream in the outer using block is guaranteed to be alive the whole time.

|

Collectives™ on Stack Overflow

C# Threading/Lock confusion

3 Answers 3

3 Comments

Comments

8 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

3 Comments

Comments

8 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related