cannot destroy thread

[ratmalwer]

The enclosed testproject runs nicely.
It starts a thread.
it stops the thread when the user hits the stop-button.
and it starts a new thread hitting the start-button again.

But it does not clean up the thread or threadid, even when I call destroy.
And does not start the old thread hitting the StartAgain-button without destroying the thread (probably because the thread has terminated).

Where am I wrong? I do not want to spoil memory or get into trouble by unused threads.

[rvk]

A few notes.
Don't use TThread.Start and TThread.Resume. They are deprecated.

The effect of this method is currently the same as calling TThread.Resume after creating a thread in a suspended state. This method was added for Delphi-compatibility, where it was introduced after TThread.Suspend and TThread.Resume were deprecated.

https://www.freepascal.org/docs-html/rtl/classes/tthread.start.html
https://www.freepascal.org/docs-html/rtl/classes/tthread.resume.html

You are using Resume for the StartAgain. That's not going to work. Even if Resume works... you didn't suspend the thread. When pressing Stop, the thread is ended... period. The Execute is exited. So there is no way to resume it.

Why are you using a global variable MyThreadAbort to abort the thread. Don't do that. Use Terminate. In that case the variable TThread.Terminated is set and the while loop will end. You can also check TThread.Terminated yourself so you also don't need that fAbortConfirmed. So remove fAbort and fAbortConfirmed from the thread entirely. You can probably remove that fActive too.

Why did you comment out the MyThread.free line in btnStopClick?
If you stop the thread you should use MyThread.WaitFor; followed by MyThread.Free; to completely free the MyThread. You can then create the thread again in btnStartAgainClick.

I would even FreeAndNil the thread so you can use Assigned() to check that you don't press that button twice.
So you only need btnStartClick() and btnStopClick().

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

1. procedure TForm1.btnStartClick(Sender: TObject);
2. begin
3.   if Assigned(MyThread) then
4.   begin
5.     ShowMessage('Thread is already running');
6.     exit;
7.   end;
8.   MyThread := TMyThread.Create(True);
9.   MyThread.OnShowStatus := @ShowStatus;
10.   MyThread.OnCheckActiv := @CheckActiv;     //wichtig... aktiviert diesen Event
11.   MyThread.OnCheckAbortConfirmed := @CheckAbortConfirmed;
12.   //wichtig... aktiviert diesen Event
13.   MyThread.Start;
14. end;
15.  
16.  
17. procedure TForm1.btnStopClick(Sender: TObject);
18. begin
19.   if not Assigned(MyThread) then
20.   begin
21.     ShowMessage('Thread was NOT running');
22.     exit;
23.   end;
24.   MyThread.Terminate;
25.   MyThread.WaitFor;
26.   MyThread.Free;
27.   MyThread := nil;
28. end;
29.  
30. procedure TForm1.btnDestroyClick(Sender: TObject);
31. begin
32.   if Assigned(MyThread) then
33.   begin
34.     MyThread.Terminate;
35.     MyThread.WaitFor;
36.     MyThread.Free;
37.     MyThread := nil;
38.   end;
39. end;

And instead, you might want to put the code from btnDestroyClick in TForm1.Destroy in case the application is closed without stopping the thread.

[ratmalwer]

Thanks rvk for this analysis and clear advises.
I will go that way.

What I did not know, is that i have to set Mythread := nil so I tryed to use destroy and skipt the Free...

[rvk]

The only reason for setting the myThread to nil is that when you want to recreate it, you can check with assigned if the thread is already created/running. Otherwise (when pressing start) assigned is still true (even when you already freed it) and you think there is still an instance running. Also in the check for stop you would think there is a thread running while it is already stopped, calling it again will crash you program. So assigning nil is very useful if you're going to reuse the thread-pointer.

Assigned is actually just a check for nil.
https://www.freepascal.org/docs-html/rtl/system/assigned.html

If you don't need the mythread anymore, you can leave out the := nil. For instance, in TForm1.Destroy, when you close the thread if it exists, you can leave the := nil out because after the Destroy of the form, you're not doing anything with mythread anyway. So the value pointing to nothing doesn't matter anymore because you're never going to check it with assigned anymore.

[PascalDragon]

A few notes.
Don't use TThread.Start and TThread.Resume. They are deprecated.

TThread.Start is not deprecated, but only supposed to be used once for a thread which had been created with CreateSuspended set to true (as the documentation states). Only TThread.Resume and TThread.Suspend are deprecated.

[rvk]

Woops, yes, sorry, I did mean Suspend and it's counterpart Resume.

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

1. procedure Resume; deprecated;
2. procedure Suspend; deprecated;

Copied and mentioned the wrong one because Start was later introduced as substitute for Resume.

Thnx for the correction.