I could use some assistance with a C# project.
Overview of the project: I'm working on a multithreading practice thing in Visual Studio 2015. It has two methods: one called Producers that creates unique messages and pushes them to a queue, and another called Consumers that writes a queued message to the console and removes it from the queue. The methods also sleep briefly to simulate lengthy processes. The messages are created using a class called Message that overrides ToString() so that calling the object returns a string containing the message's unique ID, the current time, and a unique string.
The Problem: When I run it, it always makes and consumes exactly the provided amount of messages, but in the printed messages the strings are not unique. Sometimes it clones two or more of the same message. The only reason for this happening that I can think of is that my lock statement is not working the way I think it should, and two tasks are making a message before one can increment msgProduced. I've been doing reading on the lock statement but I can't seem to figure out why it isn't locking the resource like it should. IF that is indeed the problem.
using System;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;
namespace MultiThreading
{
class TaskProgram
{
// number of messages to print
private const int MessageCount = 1000;
// number of threads should be half the number of processors
private static int threadCount = Environment.ProcessorCount / 2;
// keep track of the number of messages produced and consumed
private static int msgProduced = 0;
private static int msgConsumed = 0;
// queue (FIFO) to hold messages
static Queue<Message> queue = new Queue<Message>();
// object to lock on
private static readonly object MySharedResourceLock = new object();
static void Main(string[] args)
{
// task array to hold threads
Task[] producers = new Task[threadCount];
// runs all the tasks in the array
for (int i = 0; i < producers.Length; i++)
{
producers[i] = Task.Run(() => Producers(MessageCount));
}
Task[] consumers = new Task[threadCount];
for (int i = 0; i < consumers.Length; i++)
{
consumers[i] = Task.Run(() => Consumers(MessageCount));
}
// wait for all the tasks in the task arrays to finish before proceeding
Task.WaitAll(producers);
Task.WaitAll(consumers);
// write the total messages produced/consumed (should be MessageCount), and the final count of the queue (should be 0) to the console
Console.WriteLine("Total messages produced: " + msgProduced + " | Total messages consumed: " + msgConsumed + " | Final Queue Count: " + queue.Count);
// keep the program from exiting so we can see the results
Console.ReadKey();
}
// produce messages and push them to the queue, takes the number of messages to be produced as a parameter
static void Producers(int tasksForProducer)
{
// object that contains a random time in milliseconds
Random rand = new Random(DateTime.Now.Millisecond);
// loop to produce the requested amount of messages
for (int i = 0; i < tasksForProducer; ++i)
{
/* changes to the queue are destructive, so these actions should be behind a lock
the lock statement tries to acquire the shared resource object
if another lock has already acquired the object, it waits until */
lock (MySharedResourceLock)
{
// create a new message, feed it msgProduced to use as a unique ID
Message message = new Message(msgProduced);
// add the message to the queue
queue.Enqueue(message);
// increment msgProduced
msgProduced++;
}
// wait for 23-97ms
Thread.Sleep(rand.Next(23, 97));
// exit the loop when the number of messages is equal to the number of messages to write
if (msgProduced == tasksForProducer)
return;
}
}
// Consumers method loops once for each task
static void Consumers(int tasksForConsumer)
{
Random rand = new Random(DateTime.Now.Millisecond);
for (int i = 0; i < tasksForConsumer; ++i)
{
lock (MySharedResourceLock)
{
try
{
Message msg = null;
// make msg equal to the Message object in the queue, then dequeue it (the Message object)
msg = queue.Dequeue();
// if msg isn't null, write it to the console
if (null != msg)
Console.WriteLine(msg);
// increment msgConsumed
msgConsumed++;
}
catch
{
}
}
Thread.Sleep(rand.Next(17, 37));
if (msgConsumed == tasksForConsumer)
return;
}
}
// Class that produces unique messages
public class Message
{
// Instance variables
private static int count = 0;
private static DateTime currentTime;
private static string arbitraryText;
// Message constructor
public Message(int taskID)
{
count = taskID;
currentTime = DateTime.Now;
arbitraryText = $"{taskID} bottles of beer on the wall.";
}
// Index is a unique increasing integer
public int Index
{
// Shorthand/auto accessors
get
{
return count;
}
set
{
count = value;
}
}
// Timestamp is the current time and date when the message is created
public DateTime Timestamp
{
get
{
return currentTime;
}
set
{
currentTime = value;
}
}
// Text is a unique string
public string Text
{
get
{
return arbitraryText;
}
set
{
arbitraryText = value;
}
}
// Overrides default Object() method ToString() to make it do what I want it to
public override string ToString()
{
return Index.ToString("0000") + "|" + Timestamp + "|" + Text;
}
}
}
}
I would highly appreciate any advice.
Edit: I tried using a mutex instead of the lock statement and I had the same problem.