3

This is the first time to try working with Threads, I'm trying to copy a directory using a Thread, so here is what I did (After I read this post):

unit Unit1;

interface

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

type
  TForm1 = class(TForm)
    Button1: TButton;
    procedure Button1Click(Sender: TObject);
  private
    { Private declarations }
  public
    { Public declarations }
  end;
TMyThread= class(TThread)
  private
    Fsource, FDest: String;
  protected
  public
    constructor Create(Const Source, Dest: string);
    destructor Destroy; override;
    procedure Execute(); override;
  published
end;
var
  Form1: TForm1;
  MT: TMyThread;
implementation

{$R *.dfm}

{ TMyThread }

constructor TMyThread.Create(const Source, Dest: string);
begin
  Fsource:= Source;
  FDest:= Dest;
end;

destructor TMyThread.Destroy;
begin

  inherited;
end;

procedure TMyThread.Execute;
var Dir: TDirectory;
begin
  inherited;
  try
    Dir.Copy(Fsource, FDest);
  except on E: Exception do
    ShowMessage(E.Message);
  end;
end;

procedure TForm1.Button1Click(Sender: TObject);
begin
MT := TMyThread.Create('SourceFolder', 'DestinationFolder');
try
  MT.Execute;
finally
  MT.Free;
end;
end;

end.

When I click on the Button1 I get this error message:

Cannot call Start on a running or suspended thread

What's wrong here? I don't know much about threads,I even try:

MT := TMyThread.Create('SourceFolder', 'DestinationFolder');
Ilyes
  • 14,640
  • 4
  • 29
  • 55
  • Threads run in parallel to your main program. That means Thread.Execute runs asynchronously to your main program, so your 'Free' statement (attempts to) destroy the thread before it has a chance to run - hence your message – Dsm Jan 31 '18 at 10:21
  • @Dsm Thanks for the information, but even without `MT.Free;` I get the same error msg. – Ilyes Jan 31 '18 at 10:24
  • 3
    Don't call `Execute`. The framework does that. You are just executing it in the main thread. Don't call `ShowMessage` from the thread. Can't do GUI outside the main thread. – David Heffernan Jan 31 '18 at 10:32
  • 3
    Additionally to what David pointed out, you aren't calling any of the constructors of `TThread`. – nil Jan 31 '18 at 10:33
  • 3
    Furthermore, you really should avoid using global variables. And there's little point in calling `inherited` in your `Execute` since it overrides an abstract method. And don't declare an variable of type `TDirectory`. The `Copy` method is a class method. Use `TDirectory.Copy`. In case you don't understand @nil's comment, you need to add an `inherited` in your thread class constructor. – David Heffernan Jan 31 '18 at 10:34
  • Thanks a lot, very helpful comments. – Ilyes Jan 31 '18 at 10:40
  • @DavidHeffernan Should I `SetFreeOnTerminate(True);` or the thred will free automatically? – Ilyes Jan 31 '18 at 10:51
  • 3
    `Thread.FreeOnTerminate := True`, otherwise you need to keep hold of the reference and destroy it yourself. – David Heffernan Jan 31 '18 at 10:53
  • @DavidHeffernan Thanks again. – Ilyes Jan 31 '18 at 10:54
  • I'm against that free on terminate thing. Using thread pool is by magnitude better. – Victoria Jan 31 '18 at 15:05

1 Answers1

5

Thanks for all guys helps with helpful comments:

unit Unit1;

interface

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

type
  TForm1 = class(TForm)
    Button1: TButton;
    procedure Button1Click(Sender: TObject);
  private
    { Private declarations }
  public
    { Public declarations }
  end;
TMyThread= class(TThread)
  private
    Fsource, FDest: String;
  protected
  public
    constructor Create(Const Source, Dest: string);
    destructor Destroy; override;
    procedure Execute(); override;
  published
end;
var
  Form1: TForm1;

implementation

{$R *.dfm}

{ TMyThread }

constructor TMyThread.Create(const Source, Dest: string);
begin
  inherited Create;
  Fsource:= Source;
  FDest:= Dest;
  Self.FreeOnTerminate := True;
end;

destructor TMyThread.Destroy;
begin
  inherited;
end;

procedure TMyThread.Execute;
begin
  try
    TDirectory.Copy(Fsource, FDest);
  except on E: Exception do
  end;
end;

procedure TForm1.Button1Click(Sender: TObject);
var MT: TMyThread;
begin
MT := TMyThread.Create('Source', 'Destination');
end;

end.
Ilyes
  • 14,640
  • 4
  • 29
  • 55
  • 1
    You better set `FreeOnTerminate` to true inside the thread constructor to avoid a leak. And skip the `MT` variable, since referencing a thread object with FreeOnTerminate = true is not allowed. – LU RD Jan 31 '18 at 12:33
  • 1
    The problem with that is that it places a constraint of consumers of the class. The other option is to create suspended and then set free on terminate from the outside, and then start the thread. – David Heffernan Jan 31 '18 at 13:32
  • Or pass a boolean with the constructor to tell whether to set FreeOnTerminate or not. – LU RD Jan 31 '18 at 15:01