How to terminate the thread correctly?

[NickFF]

I need to process a file with progress indication. I created the second thread and the boolean variable named 'stop' for breaking of the repeat loop.

Code: Pascal  [Select][+][-]

1. type
2.  
3.   { TFileThread }
4.  
5.   TFileThread = class(TThread)
6.     private
7.       status, inPath, outPath: String;
8.       n, fSize: Integer;
9.     protected
10.       procedure ShowProgress;
11.       procedure Execute; override;
12.   end;
13.  
14. var
15.   Form1: TForm1;
16.   stop: Boolean;
17.  
18. implementation
19.  
20. {$R *.lfm}
21.  
22. { TFileThread }
23.  
24. procedure TFileThread.ShowProgress;
25. begin
26.   with Form1 do
27.   case status of
28.     'progress':
29.     begin
30.       StatusBar.SimpleText := '';
31.       StatusBar.SimpleText := IntToStr(n div (fSize div 100)) + '%';  // Show progress
32.     end;
33.     'finish': StatusBar.SimpleText := 'OK';
34.     'cancel': StatusBar.SimpleText := 'Canceled';
35.   end;
36. end;
37.  
38. procedure TFileThread.Execute;
39. var
40.   inFs, outFs: TFileStream;
41.   i: Integer;
42.   NumRead, NumWritten: Word;
43.   Buf: Array[0..15] of Byte;
44. begin
45.   try
46.     inFs := TFilestream.Create(inPath, fmOpenRead);
47.     outFs := TFilestream.Create(outPath, fmCreate);
48.  
49.     fSize := inFs.Size div 16;
50.  
51.     status := 'progress';
52.     for i := 0 to 15 do Buf[i] := 0;
53.     n := 0;
54.     NumRead := 0;
55.     NumWritten := 0;
56.  
57.     repeat
58.       if stop = true then    // if STOP
59.       begin
60.         status := 'cancel';
61.         break;               // then BREAK
62.       end;
63.       NumRead := inFs.Read(Buf, 16);
64.       for i := 0 to 15 do
65.         if Buf[i] > 127 then Buf[i] := 0;
66.       NumWritten := outFs.Write(Buf, 16);
67.       Synchronize(@ShowProgress);
68.       Inc(n);
69.     until (NumRead = 0) or (NumWritten <> NumRead);
70.   finally
71.     inFs.Free;
72.     outFS.Free;
73.   end;
74.  
75.   if status = 'progress' then status := 'finish'
76.   else DeleteFile(outPath);
77.  
78.   Synchronize(@ShowProgress);
79. end;
80.  
81. { TForm1 }
82.  
83. procedure TForm1.ButtonGoClick(Sender: TObject);
84. var
85.   ThreadFile: TFileThread;
86. begin
87.   stop := false;
88.   ThreadFile := TFileThread.Create(false);
89.   with ThreadFile do
90.   begin
91.     Priority := tpNormal;
92.     FreeOnTerminate := true;
93.     inPath := Edit1.Text;
94.     outPath := 'new_' + Edit1.Text;
95.   end;
96. end;
97.  
98. procedure TForm1.ButtonStopClick(Sender: TObject);
99. begin
100.   stop := true;
101. end;
102.  

Works good, but I'm not sure - is this the correct solution or do I need to call the ThreadFile.Terminate method separately? Help me find the right solution plz.

[engkin]

Typically you create the thread suspended so you can pass any needed properties before you start the thread. Your code shows that you start the thread before passing any data:

Code: Pascal  [Select][+][-]

1. procedure TForm1.ButtonGoClick(Sender: TObject);
2. var
3.   ThreadFile: TFileThread;
4. begin
5.   stop := false;
6.   ThreadFile := TFileThread.Create(false);
7.   with ThreadFile do
8.   begin
9.     Priority := tpNormal;
10.     FreeOnTerminate := true;
11.     inPath := Edit1.Text;
12.     outPath := 'new_' + Edit1.Text;
13.   end;
14. end;

Code: Pascal  [Select][+][-]

1. procedure TForm1.ButtonGoClick(Sender: TObject);
2. var
3.   ThreadFile: TFileThread;
4. begin
5.   stop := false;
6.   ThreadFile := TFileThread.Create(True);
7.   with ThreadFile do
8.   begin
9.     Priority := tpNormal;
10.     FreeOnTerminate := true;
11.     inPath := Edit1.Text;
12.     outPath := 'new_' + Edit1.Text;
13.     Start;
14.   end;
15. end;

Using a form variable stop seems correct. You might want to try a faster alternative, by ASerge, in this thread.

Edit:
You still have to make sure the thread is finished before destroying the form.

[jamie]

Looks like a leak just waiting to happen here..

I would think one would need to free the Thread at some point?

ThreadFile.Free ?

[NickFF]

You still have to make sure the thread is finished before destroying the form.

I'm a beginner and therefore i'm going to prevent the form from closing until the thread is manually stopped or finished.

I would think one would need to free the Thread at some point?
ThreadFile.Free ?

'at some point' you mean when will the form be destroyed?
Like this:

Code: Pascal  [Select][+][-]

1. procedure TForm1.FormDestroy(Sender: TObject);
2. begin
3.   ThreadFile.Terminate;
4.   ThreadFile.Free;
5. end;
6.  

?

[engkin]

Looks like a leak just waiting to happen here..

I would think one would need to free the Thread at some point?

ThreadFile.Free ?

He used:

Code: Pascal  [Select][+][-]

1.     FreeOnTerminate := true;

[NickFF]

Thanks

[taazz]

two problems with the global stop approach,
1) there is no way to distinguish between threads, which might actually be a welcome side effect.
2) You already have the FTerminated field that becomes true when the Thread.Terminate is called.

other than that you are golden with engkin's code.

[engkin]

2) You already have the FTerminated field that becomes true when the Thread.Terminate is called.

Yes, that's the way to do it if you do not use: FreeOnTerminate := true.
The code as is with FreeOnTerminate := true makes it not safe to call Thread.Terminate.

[taazz]

2) You already have the FTerminated field that becomes true when the Thread.Terminate is called.

Yes, that's the way to do it if you do not use: FreeOnTerminate := true.
The code as is with FreeOnTerminate := true makes it not safe to call Thread.Terminate.

true but can be easily overcome with a simple onTerminate event.

[engkin]

2) You already have the FTerminated field that becomes true when the Thread.Terminate is called.

Yes, that's the way to do it if you do not use: FreeOnTerminate := true.
The code as is with FreeOnTerminate := true makes it not safe to call Thread.Terminate.

true but can be easily overcome with a simple onTerminate event.

As in ASerge's code:

Code: Pascal  [Select][+][-]

1. ..
2.   FThread.FreeOnTerminate := True;
3.   FThread.OnTerminate := @ThreadTerminated;
4.   FThread.Start;
5. ..
6.  

Code: Pascal  [Select][+][-]

1. procedure TForm1.ThreadTerminated(Sender: TObject);
2. begin
3.   FThread := nil;
4. ...

Code: Pascal  [Select][+][-]

1. procedure TForm1.FormDestroy(Sender: TObject);
2. begin
3.   if Assigned(FThread) then
4.     FThread.OnTerminate := nil;
5.   FThread.Free;

Or a different approach?

[taazz]

2) You already have the FTerminated field that becomes true when the Thread.Terminate is called.

Yes, that's the way to do it if you do not use: FreeOnTerminate := true.
The code as is with FreeOnTerminate := true makes it not safe to call Thread.Terminate.

true but can be easily overcome with a simple onTerminate event.

As in ASerge's code:

Code: Pascal  [Select][+][-]

1. ..
2.   FThread.FreeOnTerminate := True;
3.   FThread.OnTerminate := @ThreadTerminated;
4.   FThread.Start;
5. ..
6.  

Code: Pascal  [Select][+][-]

1. procedure TForm1.ThreadTerminated(Sender: TObject);
2. begin
3.   FThread := nil;
4. ...

Code: Pascal  [Select][+][-]

1. procedure TForm1.FormDestroy(Sender: TObject);
2. begin
3.   if Assigned(FThread) then
4.     FThread.OnTerminate := nil;
5.   FThread.Free;

Or a different approach?

Code: Pascal  [Select][+][-]

1. procedure TForm1.FormDestroy(Sender: TObject);
2. begin
3.   if Assigned(FThread) then begin
4.     FThread.OnTerminate := nil;
5.     FThread.Terminate;
6.     FThread.WaitFor;
7.   end;
8.  

[engkin]

Code: Pascal  [Select][+][-]

1. procedure TForm1.FormDestroy(Sender: TObject);
2. begin
3.   if Assigned(FThread) then
4.     FThread.OnTerminate := nil;
5.   FThread.Free;

Or a different approach?

Code: Pascal  [Select][+][-]

1. procedure TForm1.FormDestroy(Sender: TObject);
2. begin
3.   if Assigned(FThread) then begin
4.     FThread.OnTerminate := nil;
5.     FThread.Terminate;
6.     FThread.WaitFor;
7.   end;
8.  

You do know that FThread.Free does check that FThread is not nil, it calls Terminate and WaitFor.

[taazz]

You do know that FThread.Free does check that FThread is not nil,

to late, if the FThread is nill calling free should raise a GPF exception, there is no way to allow your app to access address 0.

it calls Terminate and WaitFor.

I know, I prefer to see them and avoid the overlap, makes debugging easier.

[engkin]

You do know that FThread.Free does check that FThread is not nil,

to late, if the FThread is nill calling free should raise a GPF exception, there is no way to allow your app to access address 0.

Why? It simply calls TObject.Free:

Code: Pascal  [Select][+][-]

1.       procedure TObject.Free;
2.  
3.         begin
4.            // the call via self avoids a warning
5.            if self<>nil then
6.              self.destroy;
7.         end;

[wadman]

if the FThread is nill calling free should raise a GPF exception, there is no way to allow your app to access address 0.

TObject(nil).Free;