14
\$\begingroup\$

After I wrote this question: Producer/Consumer programs, I realized that I had forgotten to write a version using mutexes. I have now solved the Producer/Consumer problem using mutexes and implementing the tips I received from answers on that question. It appears to be working correctly; however, unlike my other programs, which would run for a while or until the buffer was full, this version tends to interlock the producer/consumer threads, so each character is consumed immediately after it is produced. Is this because the OS tends to preempt the thread when it releases the mutex if there is another waiting for it?

class Program
{
    const int BuffSize = 10;
    const int ValuesToProduce = 95;

    static void Main(string[] args)
    {
        var producerConsumer = new MutexProducerConsumer(BuffSize, ValuesToProduce);
        Thread p = new Thread(new ThreadStart(producerConsumer.Produce));
        Thread c = new Thread(new ThreadStart(producerConsumer.Consume));
        p.Start();
        c.Start();
    }
}

This is MutexProducerConsumer:

public class MutexProducerConsumer : ProducerConsumerBase
{
    public MutexProducerConsumer(int BuffSize, int ValuesToProduce)
        : base(BuffSize, ValuesToProduce) { }

    volatile int Available = 0;
    Mutex BufferLock = new Mutex(false);
    Mutex IsFull = new Mutex(true);
    Mutex IsEmpty = new Mutex(true);

    public override void Produce()
    {
        for (int i = 0; i < _TotalNumberOfValues; i++)
        {
            while (Available == _BuffSize)
            {
                Console.WriteLine("Wait Producer:");
                IsFull.WaitOne(1000);
            }

            BufferLock.WaitOne();
            _Buffer[i % _BuffSize] = (char)(32 + i % 95);
            Available++;
            Console.WriteLine("Produced: {0}", _Buffer[i % _BuffSize]);
            BufferLock.ReleaseMutex();

            try
            {
                IsEmpty.ReleaseMutex();
            }
            catch
            {

            }
        }
    }

    public override void Consume()
    {
        for (int i = 0; i < _TotalNumberOfValues; i++)
        {
            while (Available < 1)
            {
                Console.WriteLine("Wait Consumer:");
                IsEmpty.WaitOne(1000);
            }

            BufferLock.WaitOne();
            char c = _Buffer[i % _BuffSize];
            Available--;
            Console.WriteLine("Consumed: {0}", _Buffer[i % _BuffSize]);
            BufferLock.ReleaseMutex();

            try
            {
                IsFull.ReleaseMutex();
            }
            catch
            {

            }
        }
    }
}

And this is ProducerConsumerBase:

public abstract class ProducerConsumerBase
{
    protected char[] _Buffer;
    protected int _TotalNumberOfValues;
    protected int _BuffSize;

    protected ProducerConsumerBase(int buffSize, int totalNumberOfValues)
    {
        _BuffSize = buffSize;
        _Buffer = new char[buffSize];
        _TotalNumberOfValues = totalNumberOfValues;
    }

    public abstract void Produce();
    public abstract void Consume();
}
\$\endgroup\$
11
+50
\$\begingroup\$

I'll answer your primary question first -

Why is it producing 1 char and then immediately consuming that char?

Look at the producing code carefully:

BufferLock.WaitOne();
_Buffer[i % _BuffSize] = (char)(32 + i % 95);
Available++;
Console.WriteLine("Produced: {0}", _Buffer[i % _BuffSize]);
BufferLock.ReleaseMutex();
IsEmpty.ReleaseMutex(); // Try catch removed, but throws quite often

What you're saying is:

  1. Get exclusive access to the buffer (slow)
  2. Add a character to the buffer
  3. Increment a shared field (in a potentially dangerous way)
  4. Release the buffer (slow)
  5. Potentially throw and catch an exception

At the same time your consumer is doing the analogous:

  1. Get exclusive access to the buffer (slow)
  2. Get a character from the buffer
  3. Decrement a shared field (in a potentially dangerous way)
  4. Release the buffer (slow)
  5. Potentially throw and catch an exception

If you start that work at the same time, you're going to end up in a situation where the consumer is first waiting for the producer to release the buffer, then the producer will be waiting for the consumer to release it on every iteration. The threads are basically playing tennis with the Mutex - back and forth, back and forth. Don't forget that acquiring and releasing a Mutex takes a pretty long time - a couple of microseconds - which might be contributing to the 1 produced, 1 consumed pattern as I think the code will be spending most of its time waiting for the mutex operations.

Notes on the implementation

Most importantly (and I think you know this) a Mutex is utterly overkill for inter thread synchronisation.

A Mutex can only be released by the thread that acquired it. So, the consuming thread will never be able to release the IsFull mutex because it will only ever be acquired by the producing thread. The same is true for the IsEmpty mutex - the producing thread will never be able to release it. I'm sure that you noticed the exception but rather than trying to figure out why it was happening, you've caught and ignored the exception. Having a reason to ignore all exceptions from an operation is quite rare - seeing an empty catch with no comment is usually a sign that something's not right.

Locks/mutexes etc. are expensive - minimize where possible and always hold the lock for the least amount of time possible.

Mutex implements IDisposable, as your MutexProducerProducerConsumer class holds references to them at the class level it should also implement IDisposable.

Your waits are just spin blocking. IsEmpty.WaitOne(1000) will wait up to one second to acquire the mutex.

while (Available < 1)
{
    Console.WriteLine("Wait Consumer:");
    IsEmpty.WaitOne(1000);
}

Your consumer will acquire the mutex and never release it. So your code is basically:

while (Available < 1)
{
    Console.WriteLine("Wait Consumer:");
}

Producer/consumer is more easily implemented with a queue of work. It's weird that your consumer has to know how many things the producer will create.

Some general style points:

This was mentioned in your previous question. I don't think you can actually fall foul of this here, but it's worth remembering that ++ and -- aren't atomic operations.

If the type of the RHS of an assignment is obvious, use var.

You don't need to create an explicit delegate here either:

Thread p = new Thread(new ThreadStart(producerConsumer.Produce));

can be more concisely written as:

var p = new Thread(producerConsumer.Produce);
\$\endgroup\$
  • 1
    \$\begingroup\$ @hosch250 A handy way to signal the end of items is to use a poison pill--a value that won't appear normally such as '\0' here. \$\endgroup\$ – David Harkness Nov 29 '14 at 2:58
7
\$\begingroup\$

Since I wrote this post, I have learned a great many things, including that R# is very helpful when learning better programming skills.

  1. My naming is off. I should name my method parameters using camelCase, so BuffSize will become buffSize, etc. This will help the readability of the code because my brain will automatically know what group a name is in.

    Private fields should be named with _camelCase, so Available will become _available.

  2. I can make my Mutexes readonly, which will prevent me from assigning them from any instance except the constructor or place of declaration. This will guarantee that they will never be reassigned to incorrect values by accident.

  3. I should use var instead of the type name when declaring variables. It will reduce the amount of text to read, and I can tell what type is being assigned by the value assigned to the variable. This is not always possible, however, such as if I created a variable and did not assigned it immediately or assigned it with null.

  4. c is an unused variable that should be removed. The alternative would be to use it in the following line, rather than the direct value:

    Console.WriteLine("Consumed: {0}", Buffer[PosConsumer % BuffSize]);
    
  5. I originally wrote this well before C# 6.0 came out, but if I were to re-write it today, I would use C# 6.0's string interpolation feature. This would allow me to replace:

    Console.WriteLine("Consumed: {0}", Buffer[PosConsumer % BuffSize]);
    

    With:

    Console.WriteLine($"Consumed: {Buffer[PosConsumer % BuffSize]}");
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.