Terminate a 'sleeping' thread

Question

I have a task I want to run in the background and not interrupt the GUI thread to check for new program versions.

When the application starts, it immediately queues a thread to wait 15 seconds then execute the remainder of the code. If the check is manually triggered before the 15 seconds are up, the existing automatic thread should be terminated. If the application is closed at any point, any remaining threads should be terminated as soon as possible.

I use 2 minutes in the example below for easier debugging.

My issue right now is that no combination that I've tried using WaitFor, Terminate, FreeOnTerminate, and OnTerminated will get the desired result. Either Destroy isn't called and I get memory leaks, the application hangs when terminating a thread, or I get Cannot terminate externally created thread exceptions.

Thread code

unit unCheckThread;

interface

uses SysUtils, Classes, SyncObjs, Dialogs;

type
  TCheckThread = class(TThread)
  private
    FDelayEvent: TEvent;
    FDelay: Integer;
  public
    constructor Create(const ADelay: Integer);
    destructor Destroy; override;
    procedure Execute; override;
    procedure TerminatedSet; override;
  end;

implementation

{ TCheckThread }

constructor TCheckThread.Create(const ADelay: Integer);
begin
  inherited Create(True);
  FreeOnTerminate := False;

  FDelay := ADelay;
  FDelayEvent := TEvent.Create(nil, True, False, '');
end;

destructor TCheckThread.Destroy;
begin
  FDelayEvent.Free;

  inherited;
end;

procedure TCheckThread.Execute;
begin
  FDelayEvent.WaitFor(MSecsPerSec * FDelay);

  { if another thread has checked while waiting for the delay, cancel this check }
  if Terminated then Exit;

  { some long running code }
  Sleep(10000);

  if Terminated then Exit;

  Synchronize(
    procedure()
    begin
      MessageDlg('Thread completed', mtConfirmation, [mbOK], 0);
    end);
end;

procedure TCheckThread.TerminatedSet;
begin
  FDelayEvent.SetEvent;
end;

end.

UI

unit Unit1;

interface

uses
  Winapi.Windows, Winapi.Messages, System.SysUtils, System.Variants, System.Classes, Vcl.Graphics,
  Vcl.Controls, Vcl.Forms, Vcl.Dialogs, unCheckThread, Vcl.StdCtrls;

type
  TForm1 = class(TForm)
    Button1: TButton;
    Button2: TButton;
    procedure Button1Click(Sender: TObject);
    procedure Button2Click(Sender: TObject);
    procedure FormClose(Sender: TObject; var Action: TCloseAction);
    procedure FormCreate(Sender: TObject);
  private
    { Private declarations }
    AThread: TCheckThread;
    procedure onterminate(Sender: TObject);
  public
    { Public declarations }
  end;

var
  Form1: TForm1;

implementation

{$R *.dfm}

procedure TForm1.Button1Click(Sender: TObject);
begin
  if Assigned(AThread) then begin
    AThread.Terminate;
//    AThread.WaitFor; // ?
  end;

  AThread := TCheckThread.Create(0); // start immediately
  AThread.OnTerminate := onterminate;
  AThread.Start;
end;

procedure TForm1.Button2Click(Sender: TObject);
begin
  Close;
end;

procedure TForm1.FormClose(Sender: TObject; var Action: TCloseAction);
begin
  if Assigned(AThread) then begin
    AThread.Terminate;
//    AThread.WaitFor; // ??
  end;
end;

procedure TForm1.FormCreate(Sender: TObject);
begin
  AThread := TCheckThread.Create(SecsPerMin * 2); // wait for 2 mintues before starting
end;

procedure TForm1.onterminate(Sender: TObject);
begin
  FreeAndNil(AThread);
end;

end.

Answer 1

If:

• for whatever reason, you want to keep the structure of your code ...
• when you said "If the application is closed at any point, any remaining threads should be terminated as soon as possible." -> it means that we do not really care about the result of any process being run/worked on by the thread ...

Then:

• this won't be a "clean", academic, software engineering kind of way
• this is just an alternative to simply get things done (targeting 2 objectives: "killing"/terminating a thread and avoiding memory leaks)

1. Keep your line of code below.

   FreeOnTerminate := False;

2. "Kill"/terminate the thread (regardless of its state).

   TerminateThread(AThread.Handle, 0);

3. TForm1 has/owns AThread, and because your code may re-create AThread (in FormCreate then in Button1Click), free the thread every time right after it has been terminated (currently, you do this in Button1Click and FormClose). Just in case you do if Assigned(AThread) anywhere else, use FreeAndNil (it frees AThread and sets it to nil, this way Assigned(AThread) can correctly return False when the thread has been terminated and freed using your code structure).

   FreeAndNil(AThread);

The TerminateThread method (above) needs Winapi.Windows (which you have had in the uses list of your Unit1). It replaces AThread.Terminate.

It would look like below:

  if Assigned(AThread) then begin
    TerminateThread(AThread.Handle, 0);
    FreeAndNil(AThread);
  end;

Answer 2

You can't interrupt a Sleep(), so that is not a good choice for debugging, especially for long intervals. You should use FDelayEvent.WaitFor() instead. That way, the thread can "wake up" quickly when it is being terminated.

Also, instead of calling Synchronize() at the end of Execute(), you should use the OnTerminate event instead for any operations you need when the thread is finished. OnTerminate is already synchronized with the main UI thread.

For instance, you can use OnTerminate to set your AThread variable to nil when that thread is terminated, so that your Assigned(AThread) check doesn't fail. You are already trying to do this, however you can't safely Free a thread from inside its OnTerminate event handler, so you might consider using FreeOnTerminate=True instead, or at least delay the Free until after the handler exits.

For that matter, I would not suggest creating a delayed thread at app startup to begin with. Use a TTimer instead. When its OnTimer event is fired, THEN create a non-delayed thread. If the user triggers a manual check, simply disable the timer. This way, you don't waste resources creating a thread that just sits idle, and you don't have to worry about syncing multiple threads to each other.

With that said, try something more like this:

unit unCheckThread;

interface

uses
  Classes;

type
  TCheckThread = class(TThread)
  protected
    procedure Execute; override;
  public
    constructor Create(AOnTerminate: TNotifyEvent);
  end;

implementation

uses
  SysUtils;

{ TCheckThread }

constructor TCheckThread.Create(AOnTerminate: TNotifyEvent);
begin
  inherited Create(False);
  FreeOnTerminate := False;
  OnTerminate := AOnTerminate;
end;

procedure TCheckThread.Execute;
var
  I: Integer;
begin
  { some long running code }
  for I := 1 to 20 do
  begin
    if Terminated then Exit;
    Sleep(500);
  end;
end;

end.

unit Unit1;

interface

uses
  Winapi.Windows, Winapi.Messages, System.SysUtils, System.Variants, System.Classes, Vcl.Graphics,
  Vcl.Controls, Vcl.Forms, Vcl.Dialogs, Vcl.StdCtrls, unCheckThread;

const
  WM_FREE_THREAD = WM_APP + 1;

type
  TForm1 = class(TForm)
    Button1: TButton;
    Button2: TButton;
    Timer1: TTimer;
    procedure Button1Click(Sender: TObject);
    procedure Button2Click(Sender: TObject);
    procedure FormCreate(Sender: TObject);
    procedure FormDestroy(Sender: TObject);
    procedure Timer1Timer(Sender: TObject);
  private
    { Private declarations }
    AThread: TCheckThread;
    procedure ThreadFinished(Sender: TObject);
    procedure StartThread;
    procedure StopThread;
    procedure WMFreeThread(var Message: TMessage); message WM_FREE_THREAD;
  public
    { Public declarations }
  end;

var
  Form1: TForm1;

implementation

{$R *.dfm}

procedure TForm1.Button1Click(Sender: TObject);
begin
  StartThread;
end;

procedure TForm1.Button2Click(Sender: TObject);
begin
  Close;
end;

procedure TForm1.FormCreate(Sender: TObject);
begin
  Timer1.Interval := 120000; // wait for 2 minutes before starting
  Timer1.Enabled = True;
end;

procedure TForm1.FormDestroy(Sender: TObject);
begin
  StopThread;
end;

procedure TForm1.StartThread;
begin
  Timer1.Enabled := False;
  if not Assigned(AThread) then
  begin
    AThread := TCheckThread.Create(ThreadFinished);
    Button1.Enabled := False;
  end;
end;

procedure TForm1.StopThread;
begin
  if Assigned(AThread) then
  begin
    AThread.OnTerminate := nil;
    AThread.Terminate;
    AThread.WaitFor;
    FreeAndNil(AThread);
  end;
end;

procedure TForm1.ThreadFinished(Sender: TObject);
begin
  AThread := nil;

  // in 10.2 Tokyo and later, you can use TThread.ForceQueue() instead...
  // TThread.ForceQueue(nil, Sender.Free);
  PostMessage(Handle, WM_FREE_THREAD, 0, LPARAM(Sender));

  MessageDlg('Thread completed', mtConfirmation, [mbOK], 0);
  Button1.Enabled := True;
end;

procedure TForm1.Timer1Timer(Sender: TObject);
begin
  Timer1.Enabled := False;
  StartThread;
end;

procedure TForm1.WMFreeThread(var Message: TMessage);
begin
  TObject(Message.LParam).Free;
end;

end.