Free a TThread either automatically or manually

Question

I have a main thread and a separate thread in my program. If the separate thread finishes before the main thread, it should free itself automatically. If the main thread finishes first, it should free the separate thread.

I know about FreeOnTerminate, and I've read that you have to be careful using it.

My question is, is the following code correct?

procedure TMyThread.Execute;
begin
  ... Do some processing

  Synchronize(ThreadFinished);

  if Terminated then exit;

  FreeOnTerminate := true;
end;

procedure TMyThread.ThreadFinished;
begin
  MainForm.MyThreadReady := true;
end;

procedure TMainForm.Create;
begin
  MyThreadReady := false;

  MyThread := TMyThread.Create(false);
end;

procedure TMainForm.Close;
begin
  if not MyThreadReady then
  begin
    MyThread.Terminate;
    MyThread.WaitFor;
    MyThread.Free;
  end;
end;

Phagocyte

Answer 1

You can simplify this to:

procedure TMyThread.Execute;
begin
  // ... Do some processing
end;

procedure TMainForm.Create;
begin
  MyThread := TMyThread.Create(false);
end;

procedure TMainForm.Close;
begin
  if Assigned(MyThread) then
    MyThread.Terminate;
  MyThread.Free;
end;

Explanation:

• Either use FreeOnTerminate or free the thread manually, but never do both. The asynchronous nature of the thread execution means that you run a risk of not freeing the thread or (much worse) doing it twice. There is no risk in keeping the thread object around after it has finished the execution, and there is no risk in calling Terminate() on a thread that has already finished either.

• There is no need to synchronize access to a boolean that is only written from one thread and read from another. In the worst case you get the wrong value, but due to the asynchronous execution that is a spurious effect anyway. Synchronization is only necessary for data that can not be read or written to atomically. And if you need to synchronize, don't use Synchronize() for it.

• There is no need to have a variable similar to MyThreadReady, as you can use WaitForSingleObject() to interrogate the state of a thread. Pass MyThread.Handle as the first and 0 as the second parameter to it, and check whether the result is WAIT_OBJECT_0 - if so your thread has finished execution.

BTW: Don't use the OnClose event, use OnDestroy instead. The former isn't necessarily called, in which case your thread would maybe continue to run and keep your process alive.

Arango

Hello! FreeOnTerminate alone is not an option. On the other hand I don't want to have the thread hogging memory while the Main program runs. I'm synchronizing the boolean because I believe it guarantees that it'll either executes before MainForm.Close or after MainForm.Close. So MyThread.Terminate will only be called if FreeOnTerminate is false. Am I being wrong here? – Phagocyte

How much memory does the thread 'hog' once it's finished? If you can't say you have no cause for worrying. Measure first. But if you insist, then post a message from your thread as the last thing and free the thread in the message handler. Synchronize() is too vile to even think about whether your code would work under all circumstances. Just say no to it. – Arango

Accepting this as the solution as this is the cleanest one. Thanks to everyone for the answers! – Phagocyte

Answer 2

Have the main thread assign a handler to the worker thread's OnTerminate event. If the worker thread finishes first, then the handler can signal the main thread to free the thread. If the main thread finishes first, it can terminate the worker thread. For example:

procedure TMyThread.Execute;
begin
  ... Do some processing ...
end;

procedure TMainForm.Create;
begin
  MyThread := TMyThread.Create(True);
  MyThread.OnTerminate := ThreadFinished;
  MyThread.Resume; // or MyThread.Start; in D2010+
end;

const
  APPWM_FREE_THREAD = WM_APP+1;

procedure TMainForm.ThreadFinished(Sender: TObject);
begin
  PostMessage(Handle, APPWM_FREE_THREAD, 0, 0);
end;

procedure TMainForm.WndProc(var Message: TMessage);
begin
  if Message.Msg = APPWM_FREE_THREAD then
    StopWorkerThread
  else
    inherited;
end;

procedure TMainForm.StopWorkerThread;
begin
  if MyThread <> nil then
  begin
    MyThread.Terminate;
    MyThread.WaitFor;
    FreeAndNil(MyThread);
  end;
end;

procedure TMainForm.Close;
begin
  StopWorkerThread;
end;

Krenn

I don't like it. Instead of removing the problem you are hiding it deeper, so it is becoming more difficult to find out where the code may fail. – Painkiller

No, I'm not hiding anything. This is a much more efficient way of solving the original problem, and it works just fine. The worker thread should not be trying to free itself directly at all. Since the main thread is the one starting the thread, and needs to manage the thread anyway, it should be the one to free the thread. TThread has an OnTerminate event specifically for the purpose of notifying other code when the thread terminates, and accomplishes the EXACT same thing that the original Synchronize(ThreadFinished) call was doing... ... – Krenn

... The PostMessage() trick is just to delay the actual freeing of the thread a few milliseconds, since it cannot be performed within the OnTerminate event itself. But the worker thread is still being auto-freed when it terminates, it is just not being freed from inside the thread itself, that is the only difference. – Krenn

The OnTerminate code is executed in the context of main thread (using Synchronize procedure) and the old problem is still here. A reliable solution should post message from work thread to main thread and avoid using Syncronize and FreeOnTerminate:= True. – Painkiller

There is nothing wrong with OnTerminate running in the main thread context in this situation. The main thread will always receive the OnTerminate notification, either because the main thread is still running when the worker thread terminates, or because the main thread is stopping first and the call to WaitFor() allows the thread's Synchronize() to be processed (since the main message loop is likely already stopped by that point). If the main thread is stopping, the posted window message will be ignored, which is fine as the main thread's shutdown frees the worker thread anyway... – Krenn

... if you do not want OnTerminate to run in the main thread context, you can always override the thread's DoTerminate() method, you just have to make sure the code is thread-safe (the TWinControl.Handle property is not thread-safe) since DoTerminate() runs in the worker thread context, whereas the OnTerminate event runs in the main thread context. – Krenn

Well may be you are right and your solution is absolutely correct (VCL somehow resolves a potential deadlock) - but posting a 'Free Me' message directly from work thread to main thread looks much cleaner and better (and faster - no unnessessary context switching) – Painkiller

Just as long as you post the message to a thread-safe window (NOT to the TWinControl.Handle window), such as TApplication.Handle or the result of AllocateHWnd() (which has to be called in the main thread), then you can certainly post the message from the worker thread context. – Krenn

Answer 3

No, your code is not good (though it probably will work in 99.99% or even 100% cases). If you are planning to terminate work thread from main thread, don't set FreeOnTerminate to True (I don't see what are you trying to gain in the above code by setting FreeOnTerminate to True, it at least makes your code less understandable).

A more important situation with terminating work threads is that you are trying to close an application while work thread is in wait state. The thread will not be awaken if you just call Terminate, generally you should use additional syncronization object (usually event) to wake up the work thread.

And one more remark - there is no need for

  begin
    MyThread.Terminate;
    MyThread.WaitFor;
    MyThread.Free;
  end;

if you look at TThread.Destroy code, it calls Terminate and WaitFor, so

    MyThread.Free;

is enough (at least in Delphi 2009, have no Delphi 7 sources at hand to check).

---

Updated

Read mghie answer. Consider the following situation (better on 1 CPU system):

main thread is executing

procedure TMainForm.Close;
begin
  if not MyThreadReady then
  begin
    MyThread.Terminate;
    MyThread.WaitFor;
    MyThread.Free;
  end;
end;

it checked MyThreadReady value (it is False) and was switched off by scheduler.

Now scheduler switches to work thread; it executes

  Synchronize(ThreadFinished);

and forces scheduler to switch back to main thread. Main thread continues execution:

    MyThread.Terminate;   // no problem
    MyThread.WaitFor;     // ???
    MyThread.Free;

can you say what will happen at WaitFor? I can't (requires a deeper look into TThread sources to answer, but at first glance looks like a deadlock).

Your real error is something different - you have written an unreliable code and trying to find out is it correct or not. That is bad practice with threads - you should learn to write a reliable code instead.

As for resources - when the TThread (with FreeOnTerminate = False) is terminated the only resources that remains allocated is Windows thread handle (it does not use substantial Windows resources after thread is terminated) and Delphi TThread object in memory. Not a big cost to be on the safe side.

Painkiller

If I don't set FreeOnTerminate to true, the thread won't be freed until the Main thread finishes. Minutes or even hours can pass till the resources of the separate thread are freed - I don't want that. I don't understand what you mean in the second paragraph of your answer. In which case won't the code above work? What is that 00.01%? – Phagocyte

Answer 4

I would state that mixing models is simply not recommended. You either use FreeOnTerminate and never touch the thread again, or you don't. Otherwise, you need a protected way for the two to communicate.

Since you want fine control over the thread variable, then don't use FreeOnTerminate. If your thread finishes early, clear the local resources that the thread has consumed as you normally would, and then simply let the main thread free the child thread when the application is finished. You'll get the best of both worlds - resources freed by the child thread as soon as it can be, and no worries about thread synchronization. (And it's got the added bonus of being much simpler in design/code/understanding/support...)

Cilurzo

Answer 5

Honestly, your

... Do some processing

Is the real problem here. Is that a loop for doing something recursively? If not and, instead, thats a huge task, you should consider split this task in small procedures / functions, and put all together in the execute body, calling one after another with conditional if's to know the thread state, like:

 

While not Terminated do
 begin

  if MyThreadReady then
    DoStepOneToTaskCompletion
  else
    clean_and_or_rollback(Something Initialized?);

  if MyThreadReady then
    DoStepTwoToTaskCompletion
  else
    clean_and_or_rollback(Something Initialized?, StepOne);

  if MyThreadReady then
    DoStepThreeToTaskCompletion
  else
    clean_and_or_rollback(Something Initialized?, StepOne, StepTwo);

  Self.DoTerminate; // Not sure what to expect from that one
 end;

It is dirty, almost a hack, but will work as expected.

About FreeOnTerminate, well... just remove the declaration and always

FreeAndNil(ThreadObject);

I'm not a fan of syncronise. I like more critical sections, for the flexibility to extend the code to handle more shared data.

On the form public section, declare:

ControlSection : TRTLCriticalSection;

On form create or somewhere else before thread.create ,

InitializeCriticalSection(ControlSection);

Then, every time you write to a shared resource (including your MyThreadReady variable), do

EnterCriticalSection ( ControlSection );
  MyThreadReady := True; //or false, or whatever else
LeaveCriticalSection ( ControlSection );

Before you go (exit), call

DeleteCriticalSection ( ControlSection );

and free your thread as you always do.

Regards Rafael

Berthoud