CODEWITHSUNDEEP
c#multithreadingsemaphore
user222427
user222427

Reputation:

Semaphore WaitOne not operating correctly

I have a Semaphore that suspose to limit to 3, however, this just keeps calling as many as it wants. I'm assuming it's because I use (1000). However, when I try just () it will never pass the WaitOne i'm not sure what to do here.

private static Semaphore _pool;
_pool = new Semaphore(0, 3);
var options = new ParallelOptions();
options.MaxDegreeOfParallelism = 1;
Parallel.ForEach(urlTable.AsEnumerable(),options, drow =>
{
    using (var WCC = new MasterCrawlerClass())
    {
                ActiveThreads++;
                _pool.WaitOne(1000);
                Console.WriteLine("Active Thread #: " + ActiveThreads);
                WCC.MasterCrawlBegin(drow);
                Console.WriteLine("Done Crawling a datarow");
                ActiveThreads--;
                _pool.Release();

    }
});

Upvotes: 3

Views: 5985

Answers (2)

Brian Gideon
Brian Gideon

Reputation: 48949

There are a few problems here.

  • I suspect there is little reason to throttle concurrency in this situation in the first place.
  • You are setting initialCount to 0 in the Semaphore constructor. initialCount is the number of requests that can be granted right now.
  • You are using the overload of WaitOne that accepts a timeout parameter. When the timeout expires WaitOne will return regardless of whether a count was taken from the semaphore.
  • You are incrementing ActiveThreads before taking a count from the semaphore. This means that ActiveThreads more closely approximates the number of simultaneous worker threads for the ForEach operation. It will not tell you how many of those threads are executing MasterCrawlBegin.
  • The ++ operation on ActiveThreads is not thread-safe.
  • You set MaxDegreesOfParallelism to a value that pretty much eliminates any kind of concurrent processing.

Change your code so that it looks like this.

int ActiveThreads = 0;
int ActiveCrawls = 0;
var semaphore = new SemaphoreSlim(3, 3); 
Parallel.ForEach(urlTable.AsEnumerable(), drow => 
  { 
    int x = Interlocked.Increment(ref ActiveThreads);
    Console.WriteLine("Active Thread #: " + x); 
    semaphore.Wait();
    int y = Interlocked.Increment(ref ActiveCrawls);
    Console.WriteLine("Active Crawl #: " + y); 
    try
    {
      using (var WCC = new MasterCrawlerClass()) 
      { 
        WCC.MasterCrawlBegin(drow);
      }
    }
    finally
    {
      Interlocked.Decrement(ref ActiveCrawls);
      semaphore.Release();
      Interlocked.Decrement(ref ActiveThreads);
      Console.WriteLine("Done Crawling a datarow"); 
    }
  } 
});

And of course you can just set MaxDegreesOfParallelism = 3 and not worry about all the semaphore stuff.

Upvotes: 3

dlev
dlev

Reputation: 48596

You are miscontructing the semaphore. It should be _pool = new Semaphore(3, 3); Doing that will also eliminate the need to pass in a timeout parameter to WaitOne().

The first parameter is the initial number of requests that may be granted before blocking, so passing 0 means any subsequent calls to WaitOne() will immediately block.

Upvotes: 7

Related Questions