Problem synchronizing with ManualResetEvent

Question

I have written a method that downloads some files, and now I'm trying to make it download up to 5 files in parallel, and for the rest to wait for the previous ones to finish. I am using a ManualResetEvent for this, but when i include the syncronisation part it doesn't download anything anymore (without it it works).

Here is the code of the methods:

    static readonly int maxFiles = 5;
    static int files = 0;
    static object filesLocker = new object();
    static System.Threading.ManualResetEvent sync = new System.Threading.ManualResetEvent(true);

    /// <summary>
    /// Download a file from wikipedia asynchronously
    /// </summary>
    /// <param name="filename"></param>
    public void DoanloadFileAsync(string filename)
    {
        ...
        System.Threading.ThreadPool.QueueUserWorkItem(
            (o) =>
            {
                bool loop = true;
                while (loop)
                    if (sync.WaitOne())
                        lock (filesLocker)
                        {
                            if (files < maxFiles)
                            {
                                ++files;
                                if (files == maxFiles)
                                    sync.Reset();
                                loop = false;
                            }
                        }
                try
                {
                    WebClient downloadClient = new WebClient();
                    downloadClient.OpenReadCompleted += new OpenReadCompletedEventHandler(downloadClient_OpenReadCompleted);
                    downloadClient.OpenReadAsync(new Uri(url, UriKind.Absolute));
                    //5 of them do get here
                }
                catch
                {
                    lock (filesLocker)
                    {
                        --files;
                        sync.Set();
                    }
                    throw;
                }
            });
    }

    void downloadClient_OpenReadCompleted(object sender, OpenReadCompletedEventArgs e)
    {
        try
        {
            //but none of the 5 get here
            ...Download logic... //works without the ManualResetEvent
        }
        finally
        {
            lock (filesLocker)
            {
                --files;
                sync.Set();
            }
        }
    }

Am I doing something wrong?

It is written with Silverlight 4 for Windows Phone 7.

Edit: There is no Semaphore or SemaphoreSlim in Silverlight 4.

Answer 1

By the looks the WaitOne() is hit before you are creating the WebClient. Since all the code that calls Set() is inside the event handler or exception handler it will never hit.

Perhaps you by mistake included the WebClient code inside the thread pool thread method which should be outside of it

    System.Threading.ThreadPool.QueueUserWorkItem(
        (o) =>
        {
            bool loop = true;
            while (loop)
                if (sync.WaitOne())
                    lock (filesLocker)
                    {
                        if (files < maxFiles)
                        {
                            ++files;
                            if (files == maxFiles)
                                sync.Reset();
                            loop = false;
                        }
                    }

        });

//Have the try catch OUTSIDE the background thread.
            try
            {
                WebClient downloadClient = new WebClient();
                downloadClient.OpenReadCompleted += new OpenReadCompletedEventHandler(downloadClient_OpenReadCompleted);
                downloadClient.OpenReadAsync(new Uri(url, UriKind.Absolute));
                //5 of them do get here
            }
            catch
            {
                lock (filesLocker)
                {
                    --files;
                    sync.Set();
                }
                throw;
            }

Answer 2

It seems that you are trying to limit the number of threads that can enter your critical section, the file download, at once. Rather than trying to hand craft this, use a System.Threading.Semaphore - that's what it does!

Answer 3

What I meant in my comment was while use a slow lock when you can use Interlocked. Also it will be way more performant this way.

At most 5 downloads active in parallel:

public class Downloader
{
 private int fileCount = 0;
 private AutoResetEvent sync = new AutoResetEvent(false);

 private void StartNewDownload(object o)
 {
  if (Interlocked.Increment(ref this.fileCount) > 5) this.sync.WaitOne();

  WebClient downloadClient = new WebClient();
  downloadClient.OpenReadCompleted += downloadClient_OpenReadCompleted;
  downloadClient.OpenReadAsync(new Uri(o.ToString(), UriKind.Absolute));
 }

 private void downloadClient_OpenReadCompleted(object sender, OpenReadCompletedEventArgs e)
 {
  try
  {
   // Download logic goes here.
  }
  catch {}
  finally
  {
   this.sync.Set();
   Interlocked.Decrement(ref this.fileCount);
  }
 }

 public void Run()
 {
  string o = "url1";
  System.Threading.ThreadPool.QueueUserWorkItem(this.StartNewDownload, o);
  Thread.Sleep(100);

  o = "url2";
  System.Threading.ThreadPool.QueueUserWorkItem(this.StartNewDownload, o);

  o = "url3";
  System.Threading.ThreadPool.QueueUserWorkItem(this.StartNewDownload, o);
  Thread.Sleep(200);

  o = "url4";
  System.Threading.ThreadPool.QueueUserWorkItem(this.StartNewDownload, o);

  o = "url5";
  System.Threading.ThreadPool.QueueUserWorkItem(this.StartNewDownload, o);

  o = "url6";
  System.Threading.ThreadPool.QueueUserWorkItem(this.StartNewDownload, o);

  o = "url7";
  System.Threading.ThreadPool.QueueUserWorkItem(this.StartNewDownload, o);
  Thread.Sleep(200);

  o = "url8";
  System.Threading.ThreadPool.QueueUserWorkItem(this.StartNewDownload, o);
  Thread.Sleep(400);
 }
}