Properly disposing of class with thread

Question

I have a fairly complex multi-threaded Windows service working, but I can't figure out how to clean up correctly. Below is some [pseudo] code to show what I have. The actual code is much more complex, probably too much to copy/paste here.

Basically, I have a class Request that creates a thread to do the work. When a new request comes in the Listener, it sends it to the Processor, which creates the new Request and maintains the list of requests. If the service is stopped, I cleanup all the requests in the list. But when the Request work is done, how do I clean up that one instance of the class?

Thanks for any help!

Nelson

class Service
{
  Listener listener;
  Processor processor;

  OnStart()
  {
    processor = new Processor();
    listener = new Listener(processor);
  }

  OnStop()
  {
    listener.Dispose();
    processor.Dispose();
  }
}

class Listener
{
  Thread thread;
  bool terminate = false;

  Listener(Processor processor)
  {
    thread = new Thread(DoWork);
    thread.Start(processor);
  }

  DoWork(Processor processor)
  {
    WaitForConnection(NewConnection);
  }

 NewConnection(String data)
 {
    processor.NewRequest(data);

    if (terminate)
      return;

   WaitForConnection(NewConnection);
 }

  Dispose()
  {
    terminate = true;
    thread.Join();
  }
}

class Processor
{
  //I need to maintain this list so that when the service stops I can cleanly close down
  List<Request> requests = new List<Request>();

  NewRequest(string data)
  {
    request.Add(new Request(data));
  }

  Dispose()
  {
    //Cleanup each request
    foreach (Request request in requests)
    {
      request.Dispose();
    }
  }
}

class Request
{
  Thread thread;
  bool terminate;

  Request(string data)
  {
    while (true)
    {
      //Do some work
      Thread.Sleep(1000);

      if (doneWorking)
        break;

      if (terminate)
        return;
    }

    //We're done.  If I return this thread stops.  But how do I properly remove this Request instance from the Processor.requests list?
  }

  Dispose()
  {
    terminate = true;
    thread.Join();
  }
}

Solution 1

This is a rough sketch:

delegate void CompletedRequest(Request req);

class Processor : ITrackCompletion
{
    //I need to maintain this list so that when the service stops I can cleanly close down
    List<Request> requests = new List<Request>();

    public void NewRequest(string data)
    {
        lock(requests)
            request.Add(new Request(data), Complete);
    }

    public void Complete(Request req)
    {
        lock (requests)
            requests.Remove(req);
    }

    public void Dispose()
    {
        //Cleanup each request
        foreach (Request request in requests.ToArray())
        {
            request.Dispose();
        }
    }
}

class Request
{
    Thread thread;
    bool terminate;

    public Request(string data, CompletedRequest complete)
    {
        try
        {
            while (true)
            {
                //Do some work
                Thread.Sleep(1000);

                if (doneWorking)
                    break;

                if (terminate)
                    return;
            }
        }
        finally
        {
            //We're done.  If I return this thread stops.  But how do I properly remove this Request instance from the Processor.requests list?
            complete(this);
        }
    }

    void Dispose()
    {
        terminate = true;
        thread.Join();
    }
}

Solution 2

One possibility is to pass a callback to the request in the form of a delegate: "when you've finished processing, call me back to tell me". Then just execute the callback at the end of the request processing and let it handle the cleanup.

One thing to watch out for though: if you try to go through your list disposing things and then try to remove an item from the list in another thread, you'll get problems. You should probably keep a flag (accessed in a thread-safe way) and once you've started disposing of everything in the list, ignore any callbacks you get.

Comments

[Nelson Rothermel]

Is the delegate tied to that specific instance of Processor? In other words, if I create several instances of Processor and I pass a delegate to Request (for a method in Processor), when I call the delegate does it use the same instance with the correct list? If so, that was what I was missing. I had the method, the delegate, but I couldn't (probably shouldn't even if I could) use the delegate directly across the classes.

[Nelson Rothermel]

On thread-safety, wouldn't simply lock() solve the problem? If I try to remove one item from the list and Dispose() beat me to it, the foreach wouldn't find any items. Vice versa, the item would be removed and then Dispose() would clean up the rest. I'll give all this a try and let you know. Thanks again.

[Jon Skeet]

You'd need to lock around your whole foreach loop - which wouldn't be much use, really, as you don't care any more by the time that's finished. You could decide exactly how specific the delegate would have to be - it could be specific to each individual request, for example. Anonymous methods or lambda expressions are likely to be helpful here.

[Nelson Rothermel]

To remove an item I was looping and checking ReferenceEquals(), but then I realized I could just do list.Remove(Request). Then if I do GC.Collect(), shouldn't Dispose() be called automatically? Dispose() and the destructor aren't being called...

[Jon Skeet]

Dispose() isn't called by the GC - only the finalizer is - and you haven't shown a finalizer. However, I wouldn't suggest using finalizers. Just to check: do you definitely need to dispose of the request? What does it have that needs releasing?

[Nelson Rothermel]

That's basically what I have now. Thanks for confirming. Why do you do requests.ToArray()? Is it so that you can avoid locking? Is it faster/safer in any way? Thanks.

[Nelson Rothermel]

I didn't show the finalizers for simplicity's sake. Every article I have seen on properly implementing Dispose() suggest using finalizers and a Dispose(bool disposing) method as well. See: msdn.microsoft.com/en-us/library/fs2xkftw%28VS.71%29.aspx I don't absolutely need to dispose it immediately and don't plan on using GC.Collect() in the final code. Since I'm still learning I just wanted to verify that it does in fact get called. I set breakpoints on each (Dispose() and destructor) and nothing, even after GC.Collect(). Any ideas why?

[Nelson Rothermel]

Oh, and I actually do have a file open at the time. It doesn't have to release immediately, but soon would be good. :)

[csharptest.net]

Safer and performs better... Locking is not possible since the other thread will be removing it from my list, if I lock the list the Join() will never complete. It's a best-practice to never call out of a function while a lock is being held. Also, I can't simply ignore the lock as the collection will be modified and my foreach loop will get an exception.

[Nelson Rothermel]

I got first-hand experience on this on another part of the code. I figured out it was because of a lock() deadlock, but thanks to you I was able to easily resolve it.

[Boris B.]

Shouldn't the terminated flag be declared as volatile?

[csharptest.net]

@Boris B. Yes the volatile modifier on terminated may improve the above code; however, to be completely honest, I wouldn't structure the code this way to begin with.