HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Producer/Consumer programs

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
producerconsumerprograms

Problem

I am studying mutual exclusion in college, and we just covered the producer/consumer problem. The class does not involve writing code, but I decided to implement a bounded buffer version of this problem. I have never written a multi-threaded program before, nor have I written a program with mutual exclusion before, so I decided to request a review here.

I implemented three variations, a busy-waiting variation, a Semaphore variation, and a Monitor variation. All of these reside in a class named Program, which is needed for the threading. The Monitor variation looks as if there should be a simpler solution with fewer variables. Is this so?

This is the part of the code that never changes:

const int buffSize = 10;
static char[] buffer = new char[buffSize];
static int valuesToProduce = 95;

static void Main(string[] args)
{
    Thread p = new Thread(new ThreadStart(Program.produce));
    Thread c = new Thread(new ThreadStart(Program.consume));
    p.Start();
    c.Start();
}


This is the busy-waiting producer/consumer and their related global variable:

static int avail = 0;

static void produce()
{
    for(int i=0; i<valuesToProduce; i++)
    {
        while (avail == buffSize) { };
        buffer[i % buffSize] = (char)(32 + i % 95);
        Console.WriteLine("Produced: {0}", buffer[i % buffSize]);
        avail++;
    }
}

static void consume()
{
    for (int i = 0; i < valuesToProduce; i++)
    {
        while (avail < 1) { };
        char c = buffer[i % buffSize];
        Console.WriteLine("Consumed: {0}", buffer[i % buffSize]);
        avail--;
    }
}


This is the Semaphore implementation:

```
private static Semaphore isFull = new Semaphore(buffSize, buffSize);
private static Semaphore isEmpty = new Semaphore(0, buffSize);

static void produce()
{
for (int i = 0; i < valuesToProduce; i++)
{
isFull.WaitOne();
buffer[i % buffSize] = (char)(32 + i % 95);
Console.WriteLine("Produced: {0}", buffer[i % buf

Solution

Style

Standard C# naming convention for methods is PascalCase. For static and instance members there are more variants around but often they are prefixed with _ and/or area also PascalCase so they can be easily distinguished from local variables and parameters. Following standard naming conventions makes your code look more familiar to other C# developers.
Structure

It is all static methods and global variables which seriously hurts maintenance and makes testing the code real painful as you probably have already discovered (I guess you have managed with a lot of commenting in and out code).

The easiest way to simplify this is to provide an abstract base class which the various implementations can derive from an implement. You should also pass the buffer size and values to produce/consume as parameters to break dependencies on global variables.

Something along these lines:

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

    protected ProducerConsumerBase(int bufferSize, int totalNumberOfValues)
    {
        _Buffer = new char[buffSize];
        _TotalNumberOfValues = totalNumberOfValues;
    }
    
    public abstract void ProduceAll();
    public abstract void ConsumeAll();
}


this can then be derived like this:

public class BusyWaitingProducerConsumer : ProducerConsumerBase
{
    public BusyWaitingProducerConsumer(int bufferSize, int totalNumberOfValues)
        : base(bufferSize, totalNumberOfValues)
    {
    }
    
    public void ProduceAll()
    {
        ...
    }
    
    public void ConsumeAll()
    {
        ...
    }
}

public class SemaphoreProducerConsumer : ProducerConsumerBase
{
    public SemaphoreProducerConsumer (int bufferSize, int totalNumberOfValues)
        : base(bufferSize, totalNumberOfValues)
    {
    }
    
    public void ProduceAll()
    {
        ...
    }
    
    public void ConsumeAll()
    {
        ...
    }
}


The main method of your program then would be:

static const int BuffSize = 10;
static const int ValuesToProduce = 95;

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


And you can easily swap out the concrete implementation you want to test with without having to change or comment out a lot of code.
Bugs

Your busy-waiting implementation is broken in that you will need to at least call Interlocked.Increment and Interlocked.Decrement on avail because ++ and -- are not atomic operations - they consist of a read, an increment/decrement and a store. For example if avail is 1 and avail++ and avail-- are executed on two different threads simultaneously then the end result could be 0, 1 or 2.

Code Snippets

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

    protected ProducerConsumerBase(int bufferSize, int totalNumberOfValues)
    {
        _Buffer = new char[buffSize];
        _TotalNumberOfValues = totalNumberOfValues;
    }
    
    public abstract void ProduceAll();
    public abstract void ConsumeAll();
}
public class BusyWaitingProducerConsumer : ProducerConsumerBase
{
    public BusyWaitingProducerConsumer(int bufferSize, int totalNumberOfValues)
        : base(bufferSize, totalNumberOfValues)
    {
    }
    
    public void ProduceAll()
    {
        ...
    }
    
    public void ConsumeAll()
    {
        ...
    }
}


public class SemaphoreProducerConsumer : ProducerConsumerBase
{
    public SemaphoreProducerConsumer (int bufferSize, int totalNumberOfValues)
        : base(bufferSize, totalNumberOfValues)
    {
    }
    
    public void ProduceAll()
    {
        ...
    }
    
    public void ConsumeAll()
    {
        ...
    }
}
static const int BuffSize = 10;
static const int ValuesToProduce = 95;

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

Context

StackExchange Code Review Q#69451, answer score: 11

Revisions (0)

No revisions yet.