2

So I have a static class that is supposed to be used as a log file manager, capable of adding "messages" (strings) to a Queue object, and that will push messages out to a file. Trouble is, many different threads should be enqueueing, and that the writer needs to be async as well. Currently when I insert into the queue, I'm also checking to see if the writer is writing (bool check), if it's not, i set the bool and then start the writing, but I'm getting intermittent IO exceptions about file access, and then wierd writing behavior sometimes.

Someone want to give me a hand on this?

Improve this question

5 Answers 5

Reset to default
3

If you don't want to restructure your code dramatically like I suggested in my other answer, you could try this, which assumes your LogManager class has:

  • a static thread-safe queue, _SynchronizedQueue
  • a static object to lock on when writing, _WriteLock

and these methods:

public static void Log(string message) { LogManager._SynchronizedQueue.Enqueue(message); ThreadPool.QueueUserWorkItem(LogManager.Write(null)); } // QueueUserWorkItem accepts a WaitCallback that requires an object parameter private static void Write(object data) { // This ensures only one thread can write at a time, but it's dangerous lock(LogManager._WriteLock) { string message = (string)LogManager._SynchronizedQueue.Dequeue(); if (message != null) { // Your file writing logic here } } }

There's only one problem: the lock statement in the Write method above will guarantee only one thread can write at a time, but this is dangerous. A lot can go wrong when trying to write to a file, and you don't want to hold onto (block) thread pool threads indefinitely. Therefore, you need to use a synchronization object that lets you specify a timeout, such as a Monitor, and rewrite your Write method like this:

private static void Write() { if (!Monitor.TryEnter(LogManager._WriteLock, 2000)) { // Do whatever you want when you can't get a lock in time } else { try { string message = (string)LogManager._SynchronizedQueue.Dequeue(); if (message != null) { // Your file writing logic here } } finally { Monitor.Exit(LogManager._WriteLock); } } }
Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

2

It sounds like the queue is driving the file writing operation. I recommend that you invert the control relationship so that the writer drives the process and checks the queue for work instead.

The simplest way to implement this is to add a polling mechanism to the writer in which it checks the queue for work at regular intervals.

Alternately, you could create an observerable queue class that notifies subscribers (the writer) whenever the queue transitions from empty: the subscribing writer could then begin its work. (At this time, the writer should also unsubscribe from the queue's broadcast, or otherwise change the way it reacts to the queue's alerts.)

After completing its job, the writer then checks the queue for more work. If there's no more work to do, it goes to sleep and resume polling or goes to sleep and resubscribes to the queue's alerts.

As Irwin noted in his answer, you also need to use the thread-safe wrapper provided by the Queue class' Synchronized method or manually synchronize access to your Queue if multiple threads are reading from it and writing to it (as in SpaceghostAli's example).

Improve this answer

Comments

2

I would have just one thread doing the writes to avoid contentions, while i would use multiple threads to enqueue.

You are advised "To guarantee the thread safety of the Queue, all operations must be done through the wrapper returned by the Synchronized method." - from http://msdn.microsoft.com/en-us/library/system.collections.queue.aspx

Improve this answer

3 Comments

care to elaborate on the Synchronized method?
More details with examples here: msdn.microsoft.com/en-us/library/…
Those msdn examples are ... lackluster. Essentially, the Synchronized method returns a new Queue instance that wraps your source Queue. This new Queue instance guarantees that only one thread can read from it or write to it at any time. However, you still cannot enumerate the Queue contents safely (because the contents may change while enumerating it), though that shouldn't matter to you if all you want to do is Enqueue and Dequeue items.
2

You should synchronize around your queue. Have multiple threads send to the queue and a single thread read from the queue and write to the file.

public void Log(string entry) { _logMutex.WaitOne(); _logQueue.Enqueue(entry); _logMutex.ReleaseMutex(); } private void Write() { ... _logMutex.WaitOne(); string entry = _logQueue.Dequeue(); _logMutex.ReleaseMutex(); _filestream.WriteLine(entry); ... }
Improve this answer

Comments

1

Let me address the problem at a different level:

If your writing a business application then you'd want to focus on the business-logic portions rather than the infrastructural code, more so if this infra code is already available, tested and deployed to multiple production sites (taking care of your NFRs)

I'm sure you're aware of the existance of logging frameworks like log4net and others http://csharp-source.net/open-source/logging.

Have you given these a chance before hand-rolling out your own Logger?

Take this option to the technical architect of the enterprise you're writing for and see she thinks.

Cheers

Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.