views:

416

answers:

5

I have an application with 4 threads. (GUI, Controller, Producer, Consumer)

The GUI is self-explanatory.

The controller starts the producer and consumer threads after some intial setup.

The producer creates items and places them in a free slot in a "ring buffer"

The consumer takes items from the "ring buffer" and writes them to disk.

The producer creates items at a much higher rate than the consumer.

The consumer is IO heavy and IO bound.

Currently I am checking a variable in each ring buffer slot to determine if it can be written to.

if Slot.Free then
  Write Slot.Data To Disk
end if

I am not using lock/synclock instead I'm just reading / writing the value of the slot's "free" variable. I don't believe that is correct even though it is a volatile read/write. Is there a better method to read/write this variable? The variable is of type "integer" and is either 0 or 1.

A: 

Any time that data can be accessed by more than one thread, you have to write your code to assume that it will be accessed by more than one thread. "Murphy's Law" rules multithreaded scenarios.

For instance, if one thread is filling a slot, and the other is emptying it, you need a lock around it.

John Saunders
Actually, since he uses a boolean and there is only one producer and consumer, a lock is not strictly needed provided you set the data and the boolean in the right order. I'm not saying it's the way to go though :)
Thorarin
Is that a guarantee made by the CLR?
John Saunders
Hmm, it appears there is no equivalent of `volatile` in VB.NET. Without it, the CLR is technically free to change the execution order. I doubt it actually will, but that is no guarantee.
Thorarin
A: 

There are several ways you can do this safely.

  • If your architecture and requirements allow it, you can use custom events so one thread can simply signal a different listening thread to notify that a variables state has been changed. You do have to keep track of who is consuming what events though, and if those consumers are read-only on the variable, or read/write.
  • You can also use a simple custom wrapper class around a variable type (or use a generic) that does the lock/unlock code for you. In VB.NET, I've found that using the Monitor class to lock the private instance variable is really handy.
  • Mutexes and semaphores - .NET has a Mutex class and a Semaphore class. These both assist in controlling access to thread-shared variables. I like Mutexes because they're so easy to use, and you don't need to keep track of how many threads might have access to a given resource.

Please DO note that although some MSDN documentation claims that reading to or writing from a value-type (Integer, Double, etc) is an atomic operation, and hence "thread-safe", this is SELDOM TRUE in actual VB code. A simple statement like X = Y is NOT in fact atomic, as it you have to perform two operations here - first, loading the value of Y, and then setting the value of X. Little things like this make me lose my hair :)

Whatever method you decide to roll with, I find that liberal commenting throughout your code describing who has access to what resources at what time is invaluable - three months from now, you're not gonna remember the fine points of this code and comments really help.

Best of luck!

Mike
A: 

You could use Semaphores to solve the Producer Consumer problem:

static void Main(string[] args)
{ new Thread(Producer).Start(); new Thread(Consumer).Start(); }

static int K = 10;
static n = new Semaphore(0, K), e = new Semaphore(K, K);
static int[] buffer = new int[K];
static int _in, _out;

static void Producer()
{
    while (true)
    {
     e.WaitOne();
     buffer[_in] = produce();
     _in = (_in + 1) % buffer.Length;
     n.Release();
    }
}

static void Consumer()
{
    while (true)
    {
     n.WaitOne();
     var v = buffer[_out];
     _out = (_out + 1) % buffer.Length;
     e.Release();

     consume(v);
    }
}
Josef
What is the purpose of the "s" semaphore?
Semaphore `s` was meant to ensure that buffer operations (read/write) dont' overlap. But you are right, that's not really necessary in this case so I edited my answer accordingly. Thanks!
Josef
A: 

You can make your checks interlocked operations and eliminate any concern:

 const FREE = 0;
 const FILLING = 1;
 const READY = 2;
 const DRAINIG = 3;

Producer:

if (FREE == Interlocked.CompareExchange(ref slotFlag, FILLING, FREE))
{
   fillslot();
   old = Interlocked.Exchange(ref slotFlag, READY);
   Debug.Assert (FILLING == old);
}

Consumer:

if (READY == Interlocked.CompareExchange(ref slotFlag, DRAINIG, READY))
{
    drain_slot();
    old = Interlocked.Exchange(ref slotFlag, FREE);
    Debug.Assert( DRAINIG == old);
 }

This is lock free and quite efficient if you have many cores, many producers and/or many consumers. The problem with this, that is also the problem with using bool, is that there are no wait semantics. Both the Producer and the Consumer will have to spin looking for FREE and respectively READY slots. You can overcome this by adding 'ready' and 'free' events. Also you need to take care of ensuring the ring buffer write/read positions are properly maintained.

Remus Rusanu
+2  A: 

You mention using a ring buffer, but a (properly implemented) ring buffer would be able to determine if it's full without checking all it's elements, eliminating the need for a boolean in each slot.

I'm not used to VB.NET, but this should be a working (if crude) implementation of a ring buffer that blocks when it's full / empty on respective write / read actions.

Friend Class RingBuffer(Of T)
    Private _slots() As T
    Private _head As Integer
    Private _tail As Integer

    Private _readableSlots As Semaphore
    Private _readLock As Object
    Private _writableSlots As Semaphore
    Private _writeLock As Object

    Public Sub New(ByVal size As Integer)
        ReDim _slots(size - 1)

        _head = 0
        _tail = 0
        _readLock = New Object
        _writeLock = New Object

        _readableSlots = New Semaphore(0, size)
        _writableSlots = New Semaphore(size, size)
    End Sub

    Public Function Dequeue() As T
        Dim item As T
        _readableSlots.WaitOne()
        SyncLock _readLock
            item = _slots(_head)
            _head = (_head + 1) Mod _slots.Length
        End SyncLock
        _writableSlots.Release()
        Return item
    End Function

    Public Sub Enqueue(ByVal item As T)
        _writableSlots.WaitOne()
        SyncLock _writeLock
            _slots(_tail) = item
            _tail = (_tail + 1) Mod _slots.Length
        End SyncLock
        _readableSlots.Release()
    End Sub
End Class

Once you have that, your Producer and Consumer can be really dumb :) It's not exactly guaranteed that items are processed in-order if you have multiple consumers however:

Private _buffer As RingBuffer(Of Integer) = New RingBuffer(Of Integer)(5)

Private Sub Producer()
    Dim i As Integer = 0
    Do While True
        _buffer.Enqueue(i)
        i = i + 1
    Loop
End Sub

Private Sub Consumer()
    Do While True
        Debug.WriteLine(("Consumer A: " & _buffer.Dequeue))
        Thread.Sleep(1000)
    Loop
End Sub
Thorarin
Hi. Thanks for the example code. I noticed that you have 1 read lock and 1 write lock for *all* slots. Is there a reason to *not* have 1 read lock and 1 write lock *per* slot?
The lock is mostly there because the `_head` and `_tail` variables need to be updated. The slots are being used in a fixed order, so the Semaphores are sufficient to make sure a slot can be read or written.
Thorarin