[C#] Which System.Threading class to use?

Caporegime
Joined
18 Oct 2002
Posts
29,493
Location
Back in East London
The scenario:

I have a file writer that is accessed by many processes/threads, so I have to implement a queue for lines in case the file is locked. So I also have a thread that watches over the contents of the queue, and writes to disk at the first available chance.

To save performance, I Suspend() the thread when no items are in the queue, and Resume() when there are.

However, Suspend() is deprecated, and the suggestions are Mutex, Monitor, Semaphore or Event.

Which should I use?

TIA.
 
Use a monitor, although it sounds like you've written quite a bit of code to do something you could have handled by simply locking the file writer object (or another object while using the file writer), of course this assumes the threads accessing the writer are OK to wait for their turn to write.

How are you handling the queue? Are you locking a variable (list/dictionary/whatever) or have you implemented a proper queue system (MessageQueue probably)?
 
No, just a bog standard Queue. I've synchronised the thread that is writing to file. It works just fine, but I've noticed the deprecated warning and wanted to resolve it :)
 
Right then, well use a monitor on the queue to do what you want. Still sounds like you've overcomplicated it :p
 
I'd probably use an AutoResetEvent for this: have the queue processor thread wait on the event, processing everything in the queue whenever it is signalled. Then write an Enqueue method to wrap your queue that signals the event after placing something on the underlying queue.
 
Last edited:
Ok, I'm looking through the API's and I'm still unsure.. infact, unsure if it's even necessary anymore.. here is paraphrased what I have already.. after clearing up a bit from using a while (true) loop..

Code:
public void addMessage (string msg)
{
  this.queue.Enqueue(msg);
  this.StartThread();
}

[MethodImpl(MethodImplOptions.Synchronized)]
public void StartThread()
{
  if (this.thread == null || !this.thread.IsAlive)
  {
    this.thread = new Thread(WriteQueueToFile);
    this.thread.Start();
  }
}

private void WriteQueueToFile()
{
  try {
    StreamWriter sw = new StreamWriter(this.Path);
    while (this.queue.Count > 0) {
      sw.WriteLn((string)this.queue.Dequeue());
    }
    sw.Flush();
    sw.Close();
  }
  catch (IOException e)
  {
    // do nothing
  }
}
 
Ok, I'm looking through the API's and I'm still unsure.. infact, unsure if it's even necessary anymore.. here is paraphrased what I have already.. after clearing up a bit from using a while (true) loop..

Looks pretty good from a cursory glance - only thing I can suggest is that you could perhaps avoid having to construct the thread each time the StartThread is called i.e. once it has been created you can just call Start during subsequent invocations.
 
Can a thread Start() after, for example, Abort() is called?

The intention was that it is only instantiated if it's not already set and active.
 
Seriously, that's your implementation?

Without seeing what's in your queue class it's hard to be sure exactly what's going to happen, but I can definitely see potential for it to go wrong, most probably by ending up with items left in the queue as the thread exits. OK not a massive problem but you still have the potential to lose data if the application is shut down before another call to the addMessage method.

You need to be acting on changes to the queue itself (or another variable, maybe just a count of current items to process) rather than adding to the queue and then calling the StartThread method. You also need to check that the Enqueue method is thread safe or you'll probably endup with messages going missing there too (or at least have an opportunity for them to).

Another smaller problem with your code is you're not handling exceptions well enough, you have potential there to lock the file open if an exception is thrown when writing. Either close the writer in a finally block, or use a using statement.

Quite honestly, you seem to know just enough about this to cause yourself some major headaches.


Mick.
 
Last edited:
Seriously, that's your implementation?

Without seeing what's in your queue class it's hard to be sure exactly what's going to happen, but I can definitely see potential for it to go wrong, most probably by ending up with items left in the queue as the thread exits. OK not a massive problem but you still have the potential to lose data if the application is shut down before another call to the addMessage method.

You need to be acting on changes to the queue itself (or another variable, maybe just a count of current items to process) rather than adding to the queue and then calling the StartThread method. You also need to check that the Enqueue method is thread safe or you'll probably endup with messages going missing there too (or at least have an opportunity for them to).

Another smaller problem with your code is you're not handling exceptions well enough, you have potential there to lock the file open if an exception is thrown when writing. Either close the writer in a finally block, or use a using statement.

Quite honestly, you seem to know just enough about this to cause yourself some major headaches.


Mick.
Is that enough smug for now?

I've already posted elsewhere that I've only been using C# for a week, and thus threads like this. Anyway.. Instead of threadsafing the Enqueue method (which is an instance of System.Collections.Queue) I'll synchronise the addMessage().

As for basing it on an event or changes, that's what I was hoping as an answer to this thread.. oddly enough.. given why I'm asking in the first place.

This is one reason I prefer Smalltalk.. I don't have to faff with types, thread safe this or that, or any shenanigans, because the underlying framework doesn't let these problems bubble up like .NET or Java does.

As for the StreamWriter.. it's handle would be closed after exiting the method, surely? Or are you telling me the garbage collection is that poor in .NET?
 
Last edited:
Is that enough smug for now?
I'm not trying to be smug at all, I'm just telling you there are problems. I also get sick to death of people asking for help on this forum, then totally ignoring the advice they're given!

I've already posted elsewhere that I've only been using C# for a week, and thus threads like this.
Well sorry? I've not seen you post saying you've only been using C# for a week, do I have to do a background check on you before replying to any threads?

Anyway.. Instead of threadsafing the Enqueue method (which is an instance of System.Collections.Queue) I'll synchronise the addMessage().
Why, so that you can use the same queue class somewhere else in a non threadsafe way? What's the benefit of this?

/EDIT: after reading that bit again, OK fair enough, I'd still look at having a custom class for it though which was threadsafe.

As for basing it on an event or changes, that's what I was hoping as an answer to this thread.. oddly enough.. given why I'm asking in the first place.
If you look back, I gave you an answer which you then totally ignored.


Mick.
 
Last edited:
As for the StreamWriter.. it's handle would be closed after exiting the method, surely? Or are you telling me the garbage collection is that poor in .NET?
It probably would be OK, but you can't be sure with how you have it. This is why there are finally blocks and using statements, they make it easier to be sure.
 
I'm not trying to be smug at all, I'm just telling you there are problems. I also get sick to death of people asking for help on this forum, then totally ignoring the advice they're given!


Well sorry? I've not seen you post saying you've only been using C# for a week, do I have to do a background check on you before replying to any threads?


Why, so that you can use the same queue class somewhere else in a non threadsafe way? What's the benefit of this?

/EDIT: after reading that bit again, OK fair enough, I'd still look at having a custom class for it though which was threadsafe.


If you look back, I gave you an answer which you then totally ignored.


Mick.
I didn't ignore your advice, I replied with "I can't make things wait" and "It's the standard Queue" as in, it's the System.Collections.Queue class that I can't amend, I could override, granted, but surely this is not necessary if I synch() the addMessage() method which is the only public accessor to the queue, and there is only one instance of the class for each file. I have implemented a registry of file accessors, deliberately so because "one file - one accessor" just makes sense.

Locking the filewriter.. so that would be the StreamWriter or the whole thing (for clarification)? I don't see the advantage to this:

I can't afford to make the processes/threads wanting to access this file wait - it's just not an option due to the amount of work involved to allow them to wait (out of my hands.. it's VB6 stuff which was written by other people many moons ago :ugh: )

This is why I have a queue, to buffer messages until the stream/file is accessible.

What I have there already works, and hasn't had any issues with 150,000 messages per minute, for a week.

As for using a monitor.. I'm still unsure how that will help? The examples of Monitor I can find are all to do with blocking others acessing - I don't want blocking, I want Sleep()ing when the queue is empty, to save those critical cpu cycles. :)
 
I didn't ignore your advice, I replied with "I can't make things wait" and "It's the standard Queue" as in, it's the System.Collections.Queue class that I can't amend, I could override, granted, but surely this is not necessary if I synch() the addMessage() method which is the only public accessor to the queue, and there is only one instance of the class for each file. I have implemented a registry of file accessors, deliberately so because "one file - one accessor" just makes sense.
Like I said, fair enough, sync the addMessage method.

Locking the filewriter.. so that would be the StreamWriter or the whole thing (for clarification)? I don't see the advantage to this:
I said that was an option if you weren't bothered about each thread having to wait for it's turn. But what you'd do is have any variable object and do lock(var) { code to write here }, that'd be the simplest implementation of this class, but as said each thread would have to wait.

I can't afford to make the processes/threads wanting to access this file wait - it's just not an option due to the amount of work involved to allow them to wait (out of my hands.. it's VB6 stuff which was written by other people many moons ago :ugh: )
Yeah, don't make them wait then.

This is why I have a queue, to buffer messages until the stream/file is accessible.
Yep fine.

What I have there already works, and hasn't had any issues with 150,000 messages per minute, for a week.
Are you sure about that? Can you be sure every single message has been written?

Like I said, the most likely event is that some messages will wait until the next addMessage call before being written if the call to addMessage happens to after the loop but before the thread has completed, not a massive problem unless your program exists with a message still in the queue. Could easily be fixed by forcing each queue to be written after anything calling the addMessage has finished but before exiting the program.

As for using a monitor.. I'm still unsure how that will help? The examples of Monitor I can find are all to do with blocking others acessing - I don't want blocking, I want Sleep()ing when the queue is empty, to save those critical cpu cycles. :)
What exactly do you think the thread is going to be doing while it's monitoring that variable but not writing? I'd argue you'll probably be wasting more CPU cycles by forcing creating a new thread everytime you want to do some writing of the message queue to file.

Look here for something similar to what I mean (although it's more an example of using delegates).


The easiest way to get this safe based on what you've said though is just to make access to the queue threadsafe, remember you need to lock the variable when adding to the queue and when getting the count/popping the last item from it for it to be safe. Probably easiest to write a couple of methods to wrap around the Enqueue, Dequeue and Count of the queue and use them instead of accessing them directly.

Maybe something like (no idea if this builds but you should get the idea):
Code:
private bool QueueHasItems
{
    get
    {
        lock(queue)
        {
            return (this.queue.Count > 0);
        }
    }
}

private void Push(string msg)
{
    lock(queue)
    {
        this.queue.Enqueue(msg);
    }
}

private string Pop()
{
    lock(queue)
    {
        return (string)this.queue.Dequeue();
    }
}

private void WriteQueueToFile()
{
    try
    {
        using (StreamWriter sw = new StreamWriter(this.Path))
        {
            while (QueueHasItems) {
                sw.WriteLn(Pop());
            }
        }
    }
    catch (IOException e)
    {
    // do nothing
    }
}
Obviously using the Push method to add to the queue.


Mick.
 
I'd argue you'll probably be wasting more CPU cycles by forcing creating a new thread everytime you want to do some writing of the message queue to file.

I'd agree with Mick on this, thread startup is expensive - hence my original suggestion to trigger the queue processing by waking the thread via an event.

The easiest way to get this safe based on what you've said though is just to make access to the queue threadsafe, remember you need to lock the variable when adding to the queue and when getting the count/popping the last item from it for it to be safe. Probably easiest to write a couple of methods to wrap around the Enqueue, Dequeue and Count of the queue and use them instead of accessing them directly.

Wouldn't it be easier to just use the thread safe wrapper returned by Queue.Synchronized?
 
I wouldn't bother writing a subclass of Queue that is thread safe. Thread safety is always highly "application specific".

The framework delibrately doesn't provide thread safe collections because there is no way for Microsoft to know how it is going to be used... For instance your application *might* want to add 1000 items to the Queue as a single batch... In the proposed thread safe Queue subclass (above) this would result in 1000 lock operations. This is very very bad. Sure you could have a QueueMany() method in the subclass but that isn't in the interface for a queue, i.e. it would be "non standard".

Thread safe collections and other boilerplate classes can introduce hideous race conditions too. Have to be careful what you're doing. Like I say, thread safety like this is always very application specific.
 
I wouldn't bother writing a subclass of Queue that is thread safe. Thread safety is always highly "application specific".

The framework delibrately doesn't provide thread safe collections because there is no way for Microsoft to know how it is going to be used...

Often it would end up in the same or similar lock being made twice anyway. Once by the application for its business logic and once by the collection class... i.e. less performance. And also introduces the possibility of race conditions.
I'm not quite sure what you're trying to say here...of course thread safety is application specific, which is why you'd write the subclass specific to the needs of the application it's being used in. There are plenty of things not provided in the framework, doesn't mean there's never a need for them and you shouldn't write them yourself.
 
Back
Top Bottom