Am I using threads correctly?

[AlexQc 0]

I'm back to coding Windows applications and I'm using Delphi 10.4. Everything is working: no memory leak, no race conditions, no deadlocks... I'm pretty sure everything is OK but since it's my first time (seriously) using TThread in a Windows application I want to double check that I didn't do something bad!

Code simplified for readability:

 

type    
    TDirectoryWorkerThread = class(TThread)
        private
            Parameters: TDirectoryWorkerParams; //Record
            Target: TStrings;
            procedure AddToTarget(const Item: String);
            procedure ListFilesDir(Directory: String);
        public
            constructor Create(CreateSuspended: Boolean; Buffer: TStrings; Params: TDirectoryWorkerParams; OnTerminateEvent: TNotifyEvent = nil); overload;
            destructor Destroy(); override;
            procedure Execute(); override;
    end;

    TFormMain = class(TForm)
        procedure FormDestroy(Sender: TObject);
        private
            DirWorker: TDirectoryWorkerThread;
            function DirWorkerBusy(): Boolean;
            procedure DirWorkerFinished(Sender: TObject);
        protected
            procedure WndProc(var Message: TMessage); override;

implementation

const
    APPWM_FREE_DIRWORKER = WM_APP + 1;

constructor TDirectoryWorkerThread.Create(CreateSuspended: Boolean; Buffer: TStrings; Params: TDirectoryWorkerParams; OnTerminateEvent: TNotifyEvent = nil);
begin
    inherited Create(CreateSuspended);

    if Assigned(OnTerminateEvent) then
        Self.OnTerminate := OnTerminateEvent;

    //Do more initialization...
    Self.Target := Buffer;
end;

destructor TDirectoryWorkerThread.Destroy();
begin
    Self.Parameters.AllowedExtensions.Free; //TStringList

    inherited;
end;

procedure TDirectoryWorkerThread.Execute;
begin
    Self.FreeOnTerminate := False;
    ListFilesDir(Self.Parameters.Directory);
end;

procedure TFormMain.DirWorkerFinished(Sender: TObject);
begin
    PostMessage(Self.Handle, APPWM_FREE_DIRWORKER, 0, 0);
end;

function TFormMain.DirWorkerBusy(): Boolean;
begin
    Result := Self.DirWorker <> nil; //Once finished the worker will send a message to be freed so no need to check for status
end;

procedure TFormMain.DirWorkerFinished(Sender: TObject);
begin
    PostMessage(Self.Handle, APPWM_FREE_DIRWORKER, 0, 0);
end;

procedure TFormMain.WndProc(var Message: TMessage);
begin
    if Message.Msg = APPWM_FREE_DIRWORKER then
        FreeAndNil(Self.DirWorker)
    else
        inherited;
end;

procedure TFormMain.FormDestroy(Sender: TObject);
begin
    if DirWorkerBusy() then
        FreeAndNil(Self.DirWorker);
end;

//Thread created on a user event:
Self.DirWorker := TDirectoryWorkerThread.Create(False, ListBoxPrograms.Items, ThreadParams, DirWorkerFinished);

//Thread can be cancelled like this:
if (Key = VK_ESCAPE) and DirWorkerBusy() then
    FreeAndNil(Self.DirWorker);

 

[Guest]

Self.FreeOnTerminate := False; it will be dependent of "CreateSuspended" value, then, there is better place for it.

 

on Execute event it's good always verify before sensible tasks:

 

if  NOT Terminated then // if necessary use it other places

....procede any tasks

 

 then any time you can define this value = true and avoid it to procede any task... end! (Termined not kill the thread, only flag it to end when possible)

 

warning about create objects into it, because any exception will cause stop imediatelly or propagate and crash your app. better use TRY...EXCEPT in this cases. right? that way, you can control it and fix the exception and end gracefull.

my advice: create your objects on CREATE... and destroy it on DESTROY.

 

EXECUTE is intensely used, then a little code is better to performance.

 

 

Edited February 20, 2022 by Guest

[PeterBelow 238]

Quote

I'm back to coding Windows applications and I'm using Delphi 10.4. Everything is working: no memory leak, no race conditions, no deadlocks... I'm pretty sure everything is OK but since it's my first time (seriously) using TThread in a Windows application I want to double check that I didn't do something bad!

Code simplified for readability:

I see a few problems with your code.

In general you cannot access UI controls from a background thread, but you pass ListBoxPrograms.Items as the Buffer parameter to the thread's constructor. This can work if the Add method of ListBoxPrograms.Items is implemented by sending a message to the listbox control, since the OS makes sure a message is delivered in the thread that created the control's window handle. On the other hand the VCL creates a control handle on an as needed basis, and if the ListBoxPrograms.Handle has not been created yet when the thread tries to add the first item the handle would be created in the background thread context, which would not work since this thread has no message loop.

 

So: Always access UI controls only in a Synchronized or Queued method! You did not show code for the ListFilesDir method, so perhaps you are doing that already.

 

The second point to investigate is how to safely interrupt the thread's work if it has to be killed before it has completed its work. The TThread Destructor calls Terminate and then waits for the thread to die gracefully (by leaving the Ececute method). For this to work as expected the thread's work loop has to check the Terminated property on each round and exit directly if it is true. Again this is something you would do in ListFilesDir.

 

Then there is the question of error handling in the thread's work loop. The usual pattern is to wrap the whole body of the Execute method in a try except block and either pass a trapped exception to the main thread for reporting to the user via a Synchronized method (not a queued one!), or use the Windows.MessageBox function to report it directly. That API is thread-safe, while the VCL.Dialogs methods are not.

 

 

Edited February 21, 2022 by PeterBelow

[AlexQc 0]

5 hours ago, PeterBelow said:

In general you cannot access UI controls from a background thread, but you pass ListBoxPrograms.Items as the Buffer parameter to the thread's constructor. This can work if the Add method of ListBoxPrograms.Items is implemented by sending a message to the listbox control, since the OS makes sure a message is delivered in the thread that created the control's window handle. On the other hand the VCL creates a control handle on an as needed basis, and if the ListBoxPrograms.Handle has not been created yet when the thread tries to add the first item the handle would be created in the background thread context, which would not work since this thread has no message loop.

The thread is created when the user click a button or when he drag and drop files/directory to the form so yes at that point the TListBox (ListBoxPrograms) is fully created. Since I use a "plain" Delphi TListBox I guess I'm OK? (At least it's working fine here). The thread don't know nothing about the Form (it only receive the TStrings from the form TListBox.Items).

 

5 hours ago, PeterBelow said:

So: Always access UI controls only in a Synchronized or Queued method! You did not show code for the ListFilesDir method, so perhaps you are doing that already.

In ListFilesDir I just do FindFirst/FindNext in a repeat/until and in the "until" I check for Self.Terminated. When I find something I want, I do a plain Buffer.Add (TStrings.Add) to add the string to the list. I'm not sure if it's safe since the TStrings is from a VCL component... If not, do you have an example with a Synchronized or Queued method?

 

For the rest of your comments I already handle exceptions (I handle them silently - no need to report to the user).

 

Thank you for your help!

 

 

Edited February 21, 2022 by AlexQc

[David Heffernan 2347]

13 hours ago, AlexQc said:

I guess I'm OK?

Nope, you are wrong. There's a window behind that TStrings. 

[PeterBelow 238]

14 hours ago, AlexQc said:

The thread is created when the user click a button or when he drag and drop files/directory to the form so yes at that point the TListBox (ListBoxPrograms) is fully created. Since I use a "plain" Delphi TListBox I guess I'm OK? (At least it's working fine here). The thread don't know nothing about the Form (it only receive the TStrings from the form TListBox.Items).

 

It is working in this case since TListboxStrings.Add (TListboxStrings is the TStrings descendant behind TListbox.Items) does indeed send a message (LB_ADDSTRING) to the control, and the control is visible on the form. But you should not rely on such implementation details when working in a background thread.

Using Synchronize the pattern would be something like this:

 

Synchronize(
  procedure
  begin
    Buffer.Add(LCurrentFile);
  end);

where LCurrentfile is the local variable holding the filename to add to the listbox. The code goes inside your find loop.

[Lars Fosdal 1792]

14 hours ago, AlexQc said:

an example with a Synchronized or Queued method

Have a look at https://github.com/gabr42/OmniThreadLibrary and TTask in the RTL.

[AlexQc 0]

Oh great, I knew interacting with UI elements from a TThread was bad but didn't knew how to do it the proper way. I now call the "Add" to the TString wrapped under an anonymous procedure within a "Queue" call.

 

If I wanted to re-raise in the main thread an exception that occurred during the TThread.Execute. How would I do it? I was thinking something like this would work:
 

procedure TDirectoryWorkerThread.Execute;
begin
    try
        //Do work that can raise an exception
    except
        on Exception do
        begin
            Self.Synchronize(
                procedure
                begin
                    raise;
                end
            );
        end;
    end;
end;

But I get access violations...

[Guest]

any exception in thread stop it imediatelly... the consequences... I dont know your code.

then, "Execute" will be stopped imediatelly if any exception dont treated... if "execute stop" your thread stop!

 

OnTerminate is called into (later) Destroy

type
  TMyThreadX = class(TThread)
  protected
    procedure Execute; override;
  private
    procedure MyOnTerminate(Sender: TObject);
  public
    constructor Create(ASuspended: boolean);
    destructor Destroy; override;
  end;

  TForm1 = class(TForm)
    Button1: TButton;
    Memo1: TMemo;
    procedure Button1Click(Sender: TObject);
  private
    { Private declarations }
  public
    { Public declarations }
  end;

var
  Form1: TForm1;

implementation

{$R *.dfm}
{ TMyThreadX }

constructor TMyThreadX.Create(ASuspended: boolean);
begin
  inherited Create(ASuspended);
  //
  OnTerminate := MyOnTerminate;
end;

destructor TMyThreadX.Destroy;
begin
  // ...
  inherited;
end;

procedure TMyThreadX.Execute;
var
  MyArrStringToInt: TArray<string>;
  i               : integer;
begin
  // inherited;
  MyArrStringToInt := ['1', 'hello', '3', '4']; // an exception here...
  //
  if Terminated then
    exit;
  //
  TThread.Synchronize(nil,
    procedure
    begin
      Form1.Memo1.Text := 'MyThread started...';
    end);
  //
  for var MyItem in MyArrStringToInt do
  begin
    Sleep(500); // wait me please... I want drink my coca-cola
    //
    i := StrToInt(MyItem);
  end;
  // ...
  // if none exception above...
  TThread.Synchronize(nil,
    procedure
    begin
      Form1.Memo1.Lines.Add('MyThread ended!');
    end);
end;

procedure TMyThreadX.MyOnTerminate(Sender: TObject);
begin
  TThread.Synchronize(nil,
    procedure
    begin
      //
      Form1.Memo1.Lines.Add('MyThread OnTerminate!');
      //
      if not(FatalException = nil) then // the exceptions on "thread", I think that should be solved on "thread"!
        Form1.Memo1.Lines.Add('Exception: ' + Exception(FatalException).Message);
    end);
end;

var
  MyThread: TMyThreadX;

procedure TForm1.Button1Click(Sender: TObject);
begin
  try
    MyThread.Start;
  except // dont catch it, because the thread run in another plain and it dont ended yet...
    on E: Exception do
      Memo1.Lines.Add('Exception: ' + E.Message);
  end;
  //
  // MyThread.Terminate; // it dont will be executed at all...
end;

initialization

ReportMemoryLeaksOnShutdown := true;
MyThread                    := TMyThreadX.Create(true);

finalization

MyThread.Free;

end.

now try this

  for var MyItem in MyArrStringToInt do
  begin
    Sleep(500); // wait me please... I want drink my coca-cola
    //
    try
      i := StrToInt(MyItem);
    except
      on E: Exception do
        TThread.Synchronize(nil,
          procedure
          begin
            Form1.Memo1.Lines.Add('MyThread ... Exception: !' + E.Message);
          end);
    end;
  end;

 

Edited February 23, 2022 by Guest

[mvanrijnen 123]

16 hours ago, joaodanet2018 said:

any exception in thread stop it imediatelly... the consequences... I dont know your code.

then, "Execute" will be stopped imediatelly if any exception dont treated... if "execute stop" your thread stop!

 

OnTerminate is called into (later) Destroy

now try this

............................................

 

Why not override DoTerminate?  (leave the events for users of the class (owners/creators)

 TMyClass = class(TThread)
 private
.........
  protected
     procedure DoTerminate; override;
..............

 

procedure TMyClass.DoTerminate;
begin
  {TODO -oOwner -cGeneral : My Code }
  inherited;
end;

 

Edited February 23, 2022 by mvanrijnen

[Guest]

I didn't give a solution (something definitive), because each individual (especially some xpert) will have their own. There is no right or wrong, there is something more or less suitable to the code that precedes the possible solution.
Certainly my simple code is flawed. Even because it has no use in the real world of computers.
It was only an attempt to provoke the reasoning of the author of the message.
He must find the best way to reach his goal.