9

I have a Delphi application which spawns 6 anonymous threads upon some TTimer.OnTimer event.

If I close the application from the X button in titlebar Access Violation at address $C0000005 is raised and FastMM reports leaked TAnonymousThread objects.

Which is the best way to free anonymous threads in Delphi created within OnTimer event with TThread.CreateAnonymousThread() method?

SOLUTION which worked for me:

Created a wrapper of the anonymous threads which terminates them upon being Free-ed.

type
  TAnonumousThreadPool = class sealed(TObject)
  strict private
    FThreadList: TThreadList;
    procedure TerminateRunningThreads;
    procedure AnonumousThreadTerminate(Sender: TObject);
  public
    destructor Destroy; override; final;
    procedure Start(const Procs: array of TProc);
  end;

{ TAnonumousThreadPool }

procedure TAnonumousThreadPool.Start(const Procs: array of TProc);
var
  T: TThread;
  n: Integer;
begin
  TerminateRunningThreads;

  FThreadList := TThreadList.Create;
  FThreadList.Duplicates := TDuplicates.dupError;

  for n := Low(Procs) to High(Procs) do
  begin
    T := TThread.CreateAnonymousThread(Procs[n]);
    TThread.NameThreadForDebugging(AnsiString('Test thread N:' + IntToStr(n) + ' TID:'), T.ThreadID);
    T.OnTerminate := AnonumousThreadTerminate;
    T.FreeOnTerminate := true;
    FThreadList.LockList;
    try
      FThreadList.Add(T);
    finally
      FThreadList.UnlockList;
    end;
    T.Start;
  end;
end;

procedure TAnonumousThreadPool.AnonumousThreadTerminate(Sender: TObject);
begin
  FThreadList.LockList;
  try
    FThreadList.Remove((Sender as TThread));
  finally
    FThreadList.UnlockList;
  end;
end;

procedure TAnonumousThreadPool.TerminateRunningThreads;
var
  L: TList;
  T: TThread;
begin
  if not Assigned(FThreadList) then
    Exit;
  L := FThreadList.LockList;
  try
    while L.Count > 0 do
    begin
      T := TThread(L[0]);
      T.OnTerminate := nil;
      L.Remove(L[0]);
      T.FreeOnTerminate := False;
      T.Terminate;
      T.Free;
    end;
  finally
    FThreadList.UnlockList;
  end;
  FThreadList.Free;
end;

destructor TAnonumousThreadPool.Destroy;
begin
  TerminateRunningThreads;
  inherited;
end;

End here is how you can call it:

procedure TForm1.Button1Click(Sender: TObject);
begin
  FAnonymousThreadPool.Start([ // array of procedures to execute
    procedure{anonymous1}()
    var
      Http: THttpClient;
    begin
      Http := THttpClient.Create;
      try
        Http.CancelledCallback := function: Boolean
          begin
            Result := TThread.CurrentThread.CheckTerminated;
          end;
        Http.GetFile('http://mtgstudio.com/Screenshots/shot1.png', 'c:\1.jpg');
      finally
        Http.Free;
      end;
    end,

    procedure{anonymous2}()
    var
      Http: THttpClient;
    begin
      Http := THttpClient.Create;
      try
        Http.CancelledCallback := function: Boolean
          begin
            Result := TThread.CurrentThread.CheckTerminated;
          end;
        Http.GetFile('http://mtgstudio.com/Screenshots/shot2.png', 'c:\2.jpg');
      finally
        Http.Free;
      end;
    end
  ]);
end;

No memory leaks, proper shutdown and easy to use.

Gad D Lord
  • 6,620
  • 12
  • 60
  • 106
  • 1
    'anonymous threads' - Oh great.. what have Embarcadero foisted on us now? – Martin James May 01 '12 at 18:04
  • 1
    @Martin: Nothing scary, really. It's a thread whose behavior is provided at creation time by an anonymous method. It lets you use closures when defining threads. – Mason Wheeler May 01 '12 at 18:12
  • It's continually create/destroy threads, something I've spent the last 20 years telling developers to avoid. Nevertheless, if not actually started, I can't see why there should be an AV. – Martin James May 01 '12 at 18:12
  • 2
    Gad, I'm sure that's not the *address* of the access violation; $C0000005 is the Windows exception code for access violations. Please copy and paste the actual error-message text. (You can press Ctrl+C in the dialog box to copy the entire dialog's text.) – Rob Kennedy May 01 '12 at 18:15
  • What's stopping you from using the debugger to find the location of the access violation? If you can't, maybe someone else can; post minimal code necessary for reproducing the error. (It's best if you can reproduce it by starting with a new, blank application.) – Rob Kennedy May 01 '12 at 18:21
  • Hmm.. having read the documentation, (I have D2009 - no anonymous threads. I have to say that, so far, the tears are not exactly pouring down my cheeks), then some of these threads probably are running because Valgrind reports their objects still there. – Martin James May 01 '12 at 18:23
  • Your "solution" is puzzling. First, you call `WaitFor`, and then call `Terminate`. `WaitFor` won't return until the thread has stopped, so once that's happened, why tell the thread it's time to stop? Also, this code leaks memory: if the thread terminates naturally, it removes itself from the list without getting destroyed, and the thread you wait for isn't necessarily the same one you remove. Finally, you have a race condition that can lead to out-of-bounds exceptions or further memory leaks. Simplify: Don't set `OnTerminate` at all, and in `OnDestroy`, do this: `for T in L do T.Free`. – Rob Kennedy May 02 '12 at 21:26
  • Thank you Rob. Your comments on memory leak and meaningless WaitFor() leaned me to the TAnonymousThreadPool solutuion. – Gad D Lord May 03 '12 at 12:47
  • 1
    Further issues in the new code: Instead of leaking, threads that terminate by themselves can get double-freed. Since the anonymous method has no reference to the thread object, it cannot check the `Terminated` property, so calling `Terminate` does nothing. (Calling `Free` would call `Terminate` anyway.) There's no reason for a thread-safe list since it's only ever accessed in a single thread. Have you considered using [Code Review](http://codereview.stackexchange.com/)? It provides more room to talk about code than these comments. – Rob Kennedy May 03 '12 at 14:02

3 Answers3

16

If you want to maintain and exert control over a thread's lifetimes then it must have FreeOnTerminate set to False. Otherwise it is an error to refer to the thread after it has started executing. That's because once it starts executing, you've no ready way to know whether or not it has been freed.

The call to CreateAnonymousThread creates a thread with FreeOnTerminate set to True.

The thread is also marked as FreeOnTerminate, so you should not touch the returned instance after calling Start.

And so, but default, you are in no position to exert control over the thread's lifetime. However, you could set FreeOnTerminate to False immediately before calling Start. Like this:

MyThread := TThread.CreateAnonymousThread(MyProc);
MyThread.FreeOnTerminate := False;
MyThread.Start;

However, I'm not sure I would do that. The design of CreateAnonymousThread is that the thread is automatically freed upon termination. I think I personally would either follow the intended design, or derive my own TThread descendent.

David Heffernan
  • 601,492
  • 42
  • 1,072
  • 1,490
  • I see. But I still do not get it. TAnonymousThread is a descendant of TThread. So I can maintain a list of them and try to terminate them (since the RTL does not do it automatically). When I try to terminate it says: Project mtgstudio.exe raised exception class EThread with message 'Thread Error: The handle is invalid (6)'. – Gad D Lord May 02 '12 at 13:32
  • Read the docs for the anonymous thread. You are not allowed to hold a reference to the TThread since it uses FreeOnTerminate. – David Heffernan May 02 '12 at 13:54
  • Sorry, I can't look at this project. I think I answered the question you asked. Essentially, one you start an anonymous thread you lose control over it. You can no longer reference it. – David Heffernan May 02 '12 at 14:13
  • The docs mentioned are: http://docwiki.embarcadero.com/Libraries/XE2/en/System.Classes.TThread.CreateAnonymousThread – Gad D Lord May 02 '12 at 18:55
  • 2
    You can control anonymous threads as it is an normal TThread it i just initialized as FreeOnTerminate ... as I've posted in my answear. So you should edit your answear and remove the part that says "you should not use anonymous threads" – Diego Garcia Mar 11 '13 at 17:29
9

To avoid errors using CreateAnonymousThread just set FreeOnTerminate to False before starting it.

This way you can work with the thread as you usually do without any workaround.

You can read the documentation that says that CreateAnonymousThread automatically sets FreeOnTerminate to True and this is what is causing the errors when you reference the thread.

David Heffernan
  • 601,492
  • 42
  • 1,072
  • 1,490
Diego Garcia
  • 1,134
  • 1
  • 12
  • 25
3

Make your threads watch for some kind of notification from the outside. This could be an event that gets signaled, a message sent to a window owned by the thread, a command sent over a socket that your thread listens to, or whatever other form of communication you find.

If you determine that this problem is because your threads are so-called "anonymous" threads, then a simple workaround is for you to make them be non-anonymous threads. Put the body of the anonymous function into the Execute method, and pass any captured variables to the thread class via its constructor.

Rob Kennedy
  • 161,384
  • 21
  • 275
  • 467