C# Multithreaded Proxy Checker

Question

Hey,

so got a new problem...

I'm writing a multithreaded proxychecker in c#.

I'm using BackgroundWorkers to solve the multithreading problem.

But i have problems coordinating and assigning the proxys left in queue to the runnning workers. It works most of the time but sometimes no result comes back so some proxys get 'lost' during the process.

This list represents the queue and is filled with the ids of the proxys in a ListView.

private List<int> queue = new List<int>();

private int GetNextinQueue() 
    {
        if(queue.Count > 0) 
        {
            lock (lockqueue)
            {
                int temp = queue[0];
                queue.Remove(temp);
                return temp;
            }
        }
        else 
            return -1;
    }

Above is my method to get the next proxy in queue, i'm using the lock statement to prevent race conditions but i am unsure if its enough or whether it slows the process down because it makes the other threads wait... (lockqueue is an object just used for locking)

So my question is, how is it possible that some proxys are not getting checked(even if the ping fails the checking should return something, but sometimes theres just nothing) and how i can i optimize this code for performance?

Here's the rest of the code which i consider important for this question http://pastebin.com/iJduX82b

If something is missing just write a comment

Thanks :)

Answer 1

A couple of things:

1. All accesses to the queue field need to go inside a lock (lockqueue) block -- this includes the if (queue.Count > 0) line above. It's not a question of performance: your application won't work if you don't acquire the lock wherever necessary.

2. From your pastebin, the call to RunWorkerAsync looks suspicious. Currently each BackgroundWorker shares the same arguments array; you need to give each one its own copy.

Answer 2

The check for queue.Count should be performed within the lock statement. Otberwise you may check that queue.Count > 0, but by the time you are able to enter the lock, another thread may have removed an item from the queue, and you are then going to call Remove on a possibly empty queue.

You could modify it to:

private int GetNextinQueue() 
    {
        lock (lockqueue)
        {
            if(queue.Count > 0) 
            {
                int temp = queue[0];
                queue.Remove(temp);
                return temp;
            }
            else 
                return -1;
        }
    }

Basically, if you want to guard access to a data structure with a lock, make sure that you guard ALL reads and writes to that structure for thread-safety.

Answer 3

Try this instead:

private int GetNextinQueue()  
{ 
    int ret = -1;
    lock (queue)
    {
        if (queue.Count > 0)  
        { 
            int temp = queue[0]; 
            queue.Remove(temp); 
            ret = temp; 
        } 
    } 
    return ret; 
}

I wouldn't worry about performance with this - it's true that other threads will block here if one thread holds the lock, but removing an int from a List doesn't take very long.

Also, you don't really need a lockqueue object - since queue is the object you want to lock access to, just use it.

Answer 4

If you're interested in simple elegance, use a queue:

    private Queue<int> queue = new Queue<int>();
    private int GetNextinQueue()
    {
        lock (queue) { return queue.Count > 0 ? queue.Dequeue() : -1; }
    }