CODEWITHSUNDEEP
c#.netmultithreadingidisposable
Johann Gerell
Johann Gerell

Reputation: 25581

Should a class with a Thread member implement IDisposable?

Let's say I have this class Logger that is logging strings in a low-priority worker thread, which isn't a background thread. Strings are queued in Logger.WriteLine and munched in Logger.Worker. No queued strings are allowed to be lost. Roughly like this (implementation, locking, synchronizing, etc. omitted for clarity):

public class Logger
{
    private Thread workerThread;
    private Queue<String> logTexts;
    private AutoResetEvent logEvent;
    private AutoResetEvent stopEvent;

    // Locks the queue, adds the text to it and sets the log event.
    public void WriteLine(String text);

    // Sets the stop event without waiting for the thread to stop.
    public void AsyncStop();

    // Waits for any of the log event or stop event to be signalled.
    // If log event is set, it locks the queue, grabs the texts and logs them.
    // If stop event is set, it exits the function and the thread.
    private void Worker();
}

Since the worker thread is a foreground thread, I have to be able to deterministically stop it if the process should be able to finish.

Question: Is the general recommendation in this scenario to let Logger implement IDisposable and stop the worker thread in Dispose()? Something like this:

public class Logger : IDisposable
{
    ...

    public void Dispose()
    {
        AsyncStop();
        this.workerThread.Join();
    }
}

Or are there better ways of handling it?

Upvotes: 3

Views: 976

Answers (4)

ghord
ghord

Reputation: 13797

You should be aware that if user doesn't call Dispose manually (via using or otherwise) application will never exit, as Thread object will hold strong reference to your Logger. Answer provided by supercat is much better general solution to this problem.

Upvotes: 1

supercat
supercat

Reputation: 81115

It may be a good idea to have the thread hold a WeakReference to the managing object, and periodically check to ensure that it still exists. In theory, you could use a finalizer to nudge your thread (note that the finalizer, unlike the Dispose, should not do a Thread.Join), but it may be a good idea to allow for the possibility of the finalizer failing.

Upvotes: 1

nitzmahone
nitzmahone

Reputation: 13940

That's usually the best, as long as you have a deterministic way to dispose the logger (using block on the main part of the app, try/finally, shutdown handler, etc).

Upvotes: 1

Marc Gravell
Marc Gravell

Reputation: 1062530

That would certainly work - a Thread qualifies as a resource, etc. The main benefit of IDisposable comes from the using statement, so it really depends on whether the typical use for the owner of the object is to use the object for a duration of time in a single method - i.e.

void Foo() {
    ...
    using(var obj = YourObject()) {
        ... some loop?
    }
    ...
}

If that makes sense (perhaps a work pump), then fine; IDisposable would be helpful for the case when an exception is thrown. If that isn't the typical use then other than highlighting that it needs some kind of cleanup, it isn't quite so helpful.

Upvotes: 3

Related Questions