1

I am using BackgroundWorker and inside it I am using foreach loop, inside which i create new thread, wait for it to finish, and than report progress and continue foreach loop. Here is what I am talking about:

private void DoWork(object sender, DoWorkEventArgs e) {
            var fileCounter = Convert.ToDecimal(fileNames.Count());
            decimal i = 0;
            foreach (var file in fileNames) {
                i++;
                var generator = new Generator(assembly);

                var thread = new Thread(new ThreadStart(
                        delegate() {
                            generator.Generate(file);
                        }));
                thread.SetApartmentState(ApartmentState.STA);
                thread.Start();
                while (thread.IsAlive); // critical point
                int progress = Convert.ToInt32(Math.Round(i / fileCounter * 100));
                backgroundWorker.ReportProgress(progress);
            }
        }

The problem is that memory is not being freed after thread finishes (after "critical point" line is passed). I thought that when thread is not alive, all resources associated with it will be released. But apparently this is not true. Can anyone explain to me why and what am I doing wrong. Thanks.

Vale
  • 3,258
  • 2
  • 27
  • 43
  • 2
    Why are you using a separate thread if you want to wait for it to finish? just do the generate in main thread. – Daniel May 10 '11 at 09:37
  • @Dani I can't because exception is thrown inside Generate: "The calling thread must be STA because many UI components require this..." That is why I create thread and set ApartmentState to STA explicitly. – Vale May 10 '11 at 09:41

3 Answers3

5

You managed to shut up the component telling you that you were doing something wrong. You however didn't actually fix the problem. An STA, Single Threaded Apartment, is required by components that do not support threading. So that all of its methods are called from the same thread, even if the call was made on another thread. COM takes care of marshaling the call from one thread to another. An STA thread makes that possible by pumping a message loop.

What you did however is create another thread and make calls on it, distinct from the thread on which the generator object was created. This doesn't solve the problem, it is still thread-unsafe. COM still marshals the call.

What matters a great deal is the thread on which you created the generator object. Since it is an apartment threaded object, it must be created on an STA thread. There normally is only one in a Windows app, the main thread of your program, otherwise commonly known as the UI thread. If you create it on a .NET worker thread that isn't STA, like you do here, then COM will step in and create an STA thread itself to give the component a hospitable home. This is nice but usually undesirable.

There's no free lunch here, you cannot magically make a chunk of code that explicitly says it doesn't (the ThreadingModel key in the registry) support threading behave like it does. Your next best bet is to create an STA thread and run all of the code on it, including the COM object creation. Beware that you typically have to pump a message loop with Application.Run(), many COM servers assume there's one available. Especially when they tell you that an STA thread is required. You'll notice that they do when they misbehave, deadlocking on a method call or not raising events.

Regarding your original question, this is standard .NET behavior. The garbage collector runs when it needs to, not when you think it should. You can override it with GC.Collect() but that's very rarely necessary. Although it might be a quick fix in your case, COM creates a new thread for every single file. The STA thread to give the generator a home. Use Debug + Windows + Threads to see them. These threads won't stop until the COM object is destroyed. Which requires the finalizer thread to run. Your code will also consume all available memory and bomb with OOM when there are more than two thousand files, perhaps reason enough to look for a real fix.

Hans Passant
  • 922,412
  • 146
  • 1,693
  • 2,536
  • Thank you very much for this answer. I understood most of it but I am not sure what to do. What do you mean by "Your next best bet is to create an STA thread and run all of the code on it, including the COM object creation. Beware that you typically have to pump a message loop with Application.Run(), many COM servers assume there's one available. "? Should I create a thread and run backround worker in it? – Vale May 10 '11 at 12:39
  • There's no point in creating a BGW if you already create a thread. That creates just more problems, a BGW is never STA. Check this answer for an example: http://stackoverflow.com/questions/4269800/c-webbrowser-control-in-a-new-thread/4271581#4271581 – Hans Passant May 10 '11 at 12:48
  • OK, I will analyze more and see what I can do. – Vale May 10 '11 at 12:50
  • Although that's probably not the best example since it relies on an event raised by the COM object to run code. A timer could raise an event. Don't underestimate the usefulness of an hourglass cursor. – Hans Passant May 10 '11 at 12:51
  • I use BGW because I want to display progress to user in progress bar every time one file is processed. I will see how to remove it... Probably tomorrow, client just gave me some bug to fix :) – Vale May 10 '11 at 12:57
  • Do *not* use BGW here. You are going to need help, talk to your team members. – Hans Passant May 10 '11 at 13:13
  • @Hans Passant I got rid of BGW. I am now reporting progress via callback using interface. But I still have too much memory in use. I found out that most of it is consumed by readers and writers. I don't know why are they not disposing... – Vale May 11 '11 at 09:34
1

This might be cause garbage collection is not immediate. Try to collect after the thread goes out of scope:
Edit:
You also need to implement a better way to wait for the thread to finish other then busy wait(while (thread.IsAlive);) to save CPU time, you can use AutoResetEvent.

private void DoWork(object sender, DoWorkEventArgs e) {
            var fileCounter = Convert.ToDecimal(fileNames.Count());
            decimal i = 0;
            var Event = new AutoResetEvent(false);
            foreach (var file in fileNames) {
                i++;
                var generator = new Generator(assembly);

                {
                    var thread = new Thread(new ThreadStart(
                            delegate() {
                                generator.Generate(file);
                                Event.Set();
                            }));
                    thread.SetApartmentState(ApartmentState.STA);
                    thread.Start();
                    //while (thread.IsAlive); // critical point
                    Event.WaitOne();
                }
                GC.Collect();
                int progress = Convert.ToInt32(Math.Round(i / fileCounter * 100));
                backgroundWorker.ReportProgress(progress);
            }
        }
Daniel
  • 30,896
  • 18
  • 85
  • 139
  • I have tried that already. Nothing is happening. Occupied memory is still the same. – Vale May 10 '11 at 09:50
  • Is there a chance generator sets some variable outside of the scope of the thread? – Daniel May 10 '11 at 09:54
  • Maybe, because it is creating instances of types from external assemblies. – Vale May 10 '11 at 09:59
  • Try to dispose each thing it creates – Daniel May 10 '11 at 10:07
  • AutoResetEvent helped freeing CPU, as you said, but memory is not being freed... Is there a way to explicitly free all resources that thread was using? – Vale May 10 '11 at 10:08
  • I tried setting instances to null and to use using for readers and writers. Still the same. – Vale May 10 '11 at 10:35
  • `using` for readers and writers don't close them as far as I know, you need to close them manually, like `TextReader.Close()` – Daniel May 10 '11 at 10:45
0

Do Generate method get data from UI controls?

szogun1987
  • 627
  • 6
  • 14