0

My program is a multi threaded proxy checker and whenever I return the proxy ip addresses from my method and try to echo them out I'm getting a bunch and the threads are doing it completely unintended. It's supposed to supply each thread with a line of IP addresses. Here's a screenshot of what's echoing. After this the IP variable will return and contain null.

My plagued code (bear with me, based off a public example): 

static List<String> ips = new List<String>();// this is at the start of the program class

static Random rnd = new Random();

private static String getip()
{
    if (ips.Count == 0)
    {
        return null;
    }
    return ips[rnd.Next(0, ips.Count)];
}

Also the get IP is called in a while (true) loop as it's a proxy checker, I don't think that code is too necessary.

The other code:

while (true)
            {
                string ip = getip();
                try
                {
                    using (var client = new ProxyClient(ip, user, pass))
                    {
                        Console.WriteLine(ip, user, pass);
                        client.Connect();
                        if (client.IsConnected)
                        {
                            return true;
                        }
                        else
                        {
                            client.Disconnect();
                            return false;
                        }
                    }
                }
                catch
                {
                    removeip(ip);
                }
                Thread.Sleep(30);
            }

For example, thread 1 should have 127.0.0.1 (first IP from list), thread 2, 127.0.0.2 (second IP from list) etc etc, the problem at the moment is located in the screenshot.

Edit: this is not a duplicate i didn't explain what i need properly, this note from Eric J explains what i'm trying to do, it wasn't just the random issue.

NOTE

If you want each thread to get its own unique IP rather than a random one, you'll need to do something different than pick a random IP. You can after all get the same random IP more than once (if you flip a coin twice, you might get head twice or tails twice).

A good strategy would be to start from your List<String> ips and create one thread for each entry in that list.
csharpnoobie
  • 21
  • 4

1 Answers1

0

I don't see a need to lock the code in question. List<T> is thread-safe for read access. You only need to lock it if you are adding to or modifying the list (see Thread Safety at the bottom of the MSDN entry).

The problem you are experiencing is unrelated. When you create a new Random(), the seed for the pseudo-random number generator is based on the system clock. Multiple calls in quick succession can happen on the same clock tick, meaning that they get the same number sequence.

Initialize your random outside of getip() to avoid the problem (e.g. make it a static field of your class).

static List<String> ips = new List<String>();// this is at the start of the program class

// See the discussion in comments about multithreaded access to Random()
// In particular see http://stackoverflow.com/a/19271004/141172
static Random rnd = // Get a thread safe Random instance 

private static String getip()
{
    if (ips.Count == 0)
    {
        return null;
    }
    return ips[rnd.Next(0, ips.Count)];
}

NOTE

If you want each thread to get its own unique IP rather than a random one, you'll need to do something different than pick a random IP. You can after all get the same random IP more than once (if you flip a coin twice, you might get head twice or tails twice).

A good strategy would be to start from your List<String> ips and create one thread for each entry in that list. Pass the IP it should be responsible for as a parameter.

Eric J.
  • 147,927
  • 63
  • 340
  • 553
  • Getting: Error 1 The contextual keyword 'var' may only appear within a local variable declaration & Error 2 The type or namespace name 'var' could not be found (are you missing a using directive or an assembly reference?) – csharpnoobie Jun 15 '16 at 17:07
  • I'll accept your answer & post a new question mate, thank you. your note is exactly what i'm trying to do i didn't explain it properly. @EricJ – csharpnoobie Jun 15 '16 at 17:14
  • Pass the ip as a parameter where? – csharpnoobie Jun 15 '16 at 17:28
  • I would be careful of having a static `Random`, the `Random` class is not thread safe and calling `.Next(int, int)` from multiple threads concurrently is known to break the class. – Scott Chamberlain Jun 15 '16 at 17:38
  • @ScottChamberlain: Interesting, I didn't realize that.This highly upvoted solution seems to suffer from the probability of generating Random instances with the same seed (when the threads are created and start using Random on the same tick) http://stackoverflow.com/questions/19270507/correct-way-to-use-random-in-multithread-application Do I see that right? Have you seen a better multi-thread solution? I suppose that solution could be tweaked to include the ThreadId when generating a seed. – Eric J. Jun 15 '16 at 18:08
  • @EricJ. Random is fast enough that a simple lock around the .Next( would not cause that much delay in contention. All of the methods in Random are marked vitual so it would be easy to make a derived ThreadSafeRandom class. However if you really need it to be fast, the thead local solutions you linked to are a good solution, you just need 1 extra random that generates the seeds for the new randoms in a thread safe way, but [there can be problems with that too](http://stackoverflow.com/questions/25390301). You also could use RNGCryptoServiceProvider which is thread safe, but a little overkill. – Scott Chamberlain Jun 15 '16 at 18:44