Trouble with ConcurrentStack<T> in .net c#4

This class is designed to take a list of urls, scan them, then return a list of those which does not work. It uses multiple threads to avoid taking forever on long lists.

My problem is that even if i replace the actual scanning of urls with a test function which returns failure on all urls, the class returns a variable amount of failures.

I'm assuming my problem lies either with ConcurrentStack.TryPop() or .Push(), but I cant for the life of me figure out why. They are supposedly thread safe, and I've tried locking as well, no help there.

Anyone able to explain to me what I am doing wrong? I don't have a lot of experience with multiple threads..

public class UrlValidator
{
    private const int MAX_THREADS = 10;
    private List<Thread> threads = new List<Thread>();
    private ConcurrentStack<string> errors = new ConcurrentStack<string>();
    private ConcurrentStack<string> queue = new ConcurrentStack<string>();

    public UrlValidator(List<string> urls)
    {
        queue.PushRange(urls.ToArray<string>());
    }

    public List<string> Start()
    {
        threads = new List<Thread>();

        while (threads.Count < MAX_THREADS && queue.Count > 0)
        {
            var t = new Thread(new ThreadStart(UrlWorker));
            threads.Add(t);
            t.Start();
        }

        while (queue.Count > 0) Thread.Sleep(1000);

        int runningThreads = 0;
        wh开发者_如何学JAVAile (runningThreads > 0)
        {
            runningThreads = 0;
            foreach (Thread t in threads) if (t.ThreadState == ThreadState.Running) runningThreads++;
            Thread.Sleep(100);
        }

        return errors.ToList<string>();
    }

    private void UrlWorker()
    {
        while (queue.Count > 0)
        {
            try 
            {
                string url = "";
                if (!queue.TryPop(out url)) continue;
                if (TestFunc(url) != 200) errors.Push(url);
            }
            catch
            {
                break;
            }
        }
    }

    private int TestFunc(string url)
    {
        Thread.Sleep(new Random().Next(100));
        return -1;
    }
}

---

This is something that the Task Parallel Library and PLINQ (Parallel LINQ) would be really good at. Check out an example of how much easier things will be if you let .NET do its thing:

public IEnumerable<string> ProcessURLs(IEnumerable<string> URLs)
{
    return URLs.AsParallel()
        .WithDegreeOfParallelism(10)
        .Where(url => testURL(url));
}

private bool testURL(string URL)
{
    // some logic to determine true/false
    return false;
}

Whenever possible, you should let the libraries .NET provides do any thread management needed. The TPL is great for this in general, but since you're simply transforming a single collection of items, PLINQ is well suited for this. You can modify the degree of parallelism (I would recommend setting it less than your maximum number of concurrent TCP connections), and you can add multiple conditions just like LINQ allows. Automatically runs parallel, and makes you do no thread management.

---

Your problem has nothing to do with ConcurrentStack, but rather with the loop where you are checking for running threads:

int runningThreads = 0;
while (runningThreads > 0)
{
    ...
}

The condition is immediately false, so you never actually wait for threads. In turn, this means that errors will contain errors from whichever threads have run so far.

However, your code has other issues, but creating threads manually is probably the greatest one. Since you are using .NET 4.0, you should use tasks or PLINQ for asynchronous processing. Using PLINQ, your validation can be implemented as:

public IEnumerable<string> Validate(IEnumerable<string> urls)
{ 
    return urls.AsParallel().Where(url => TestFunc(url) != 200);
}