views:

636

answers:

3

consider this code block

public void ManageInstalledComponentsUpdate()
        {
            IUpdateView view = new UpdaterForm();
            BackgroundWorker worker = new BackgroundWorker();
            Update update = new Update();
            worker.WorkerReportsProgress = true;
            worker.WorkerSupportsCancellation = true;
            worker.DoWork += new DoWorkEventHandler(update.DoUpdate);
            worker.ProgressChanged += new ProgressChangedEventHandler(view.ProgressCallback);
            worker.RunWorkerCompleted += new RunWorkerCompletedEventHandler(view.CompletionCallback);            
            worker.RunWorkerAsync();
            Application.Run(view as UpdaterForm);     
        }

It all works great but I want to understand why the objects (worker,view and update) don't get garbage collected

+4  A: 

Threads count as root objects; I don't know exactly how BackgroundWorker operates, but it seems likely that the primary thread method is going to access state on the worker instance; as such, the worker thread itself will keep the BackgroundWorker instance alive until (at least) the thread has exited.

Of course; collection also requires that all other (live) objects have de-referenced the worker object; note also that collection of stack variables can be different in debug/release, and with/without a debugger attached.

[edit] As has also been noted; the event handlers on the worker (in your code) will keep the "view" and "update" objects alive (via the delegate), but not the other way around. As long as the worker has a shorter life than the "view" and "update", you don't need to get paranoid about unsubscribing the events. I've edited the code to include a "SomeTarget" object that isonly referenced by the worker: you should see this effect (i.e. the target dies with the worker).

Re worker getting collected when the thread dies: here's the proof; you should see "worker finalized" after the worker reports exit:

using System;
using System.ComponentModel;
using System.Threading;
using System.Windows.Forms;
class Demo : Form
{
    class ChattyWorker : BackgroundWorker
    {
        ~ChattyWorker()
        {
            Console.WriteLine("Worker finalized");
        }
    }
    class SomeTarget
    {
        ~SomeTarget()
        {
            Console.WriteLine("Target finalized");
        }
        public SomeTarget()
        {
            Console.WriteLine("Target created");
        }
        public void Foo(object sender, EventArgs args)
        {
            Console.WriteLine("Foo");
        }
    }
    static void Collect(object sender, EventArgs args)
    {
        Console.WriteLine("Collecting...");
        GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced);
    }
    protected override void OnLoad(EventArgs e)
    {
        base.OnLoad(e);

        System.Windows.Forms.Timer timer = new System.Windows.Forms.Timer();
        timer.Interval = 100;
        timer.Tick += Collect;
        timer.Start();

        ChattyWorker worker = new ChattyWorker();
        worker.RunWorkerCompleted += new SomeTarget().Foo;
        worker.DoWork += delegate
        {
            Console.WriteLine("Worker starting");
            for (int i = 0; i < 10; i++)
            {
                Thread.Sleep(250);
                Console.WriteLine(i);
            }
            Console.WriteLine("Worker exiting");
        };
        worker.RunWorkerAsync();
    }
    [STAThread]
    static void Main()
    { // using a form to force a sync context
        Application.Run(new Demo());
    }
}
Marc Gravell
I'd love to know why that got down-voted...
Marc Gravell
A: 

Event handlers are references, so until you have event handler attached to the worker, it would not be considered "unreachable".

In your ComplitionCallback take care to unhook the event handlers.

Sunny
Delegates are one-way references; the events on BackgroundWorker will keep the "view" and "update" objects alive, but not the other way around. i.e. if the worker is going out of scope anyway, there is not a huge amount of benefit in unhooking the handlers. Feel free, though.
Marc Gravell
A: 

Those local variable objects are keep alive until the function exits, which is when the form exits. So null them out before call to Run, or move them to a different context.

public void ManageInstalledComponentsUpdate() {
    UpdaterForm form = new UpdaterForm();
    FireAndForgetWorker( form );
    Application.Run( form );  //does not return until form exits
}

void FireAndForgetWorker( IUpdateView view ) {
    BackgroundWorker worker = new BackgroundWorker();
    Update update = new Update();
    worker.WorkerReportsProgress = true;
    worker.WorkerSupportsCancellation = true;
    worker.DoWork += new DoWorkEventHandler(update.DoUpdate);
    worker.ProgressChanged += new ProgressChangedEventHandler(view.ProgressCallback);
    worker.RunWorkerCompleted += new RunWorkerCompletedEventHandler(view.CompletionCallback);
    worker.RunWorkerAsync();
}

A note to vsick:

Try running the following program, you will be surprised that x lives forever.

using System;

class FailsOnGarbageCollection  
{ ~FailsOnGarbageCollection() { throw new NotSupportedException(); } }

class Program{
    static void WaitForever() { while (true) { var o = new object(); } }

    static void Main(string[] args)
    {
        var x = new FailsOnGarbageCollection();
        //x = null; //use this line to release x and cause the above exception
        WaitForever();
    }
}
jyoung
That's not right. Local variables don't have be kept alive when they are not used in the rest of the function (including `this`!).
svick
Sorry vsick, but it is right. Try the program above to see why you need to null out those variables.
jyoung
Try the program in *Release mode* to see that you **don't have to** null out the variables (at least in this case, it may be appropriate e.g. for fields).
svick
The point is that the only guarantee is that the stack objects are removed as gcroots at function completion. This can optimized in some cases, but that is up to the compiler writer. It bad to rely on unspecified behavior. It may be ok to rely on it in the 1st 1000 cases, but what about case 1001?
jyoung
Well, but it doesn't actually matter whether whether those objects are gcroots. What's important is whether they (and objects “below” them in the object graph) get deallocated. And you have no guarantee when or whether that's going to happen (your 1001st case argument applies here). So, if you need to rely on that, you probably shouldn't be using managed memory at all. Nulling the references out is 1. useless because of the compiler's optimizations and 2. doesn't guarantee actual dealoccation because of GC's unpredictability.
svick
You seem a bit off topic. The question was why there was a memory leak. I have shown (even wrote a example program) that in some cases that you need to null an unused variable to allow the GC to collect it. You keep saying 'but compiler optimizations', but how do you know Mitch was not running in debug mode? You have to look at all cases, not just the one you want to pick. P.S. Do you do realize that anything that is a GCRoot can not be garbaged collected? Also note that I am saying that that this non-null field is keeping it alive. I never mention that it was guaranteed to be collected.
jyoung