Proper way to terminate a thread? lazarus / 34-Windows ---------------------------------------------------------------------- piola · 2024-06-04 15:55 Hello, I know this question has been asked before, but I'm still struggling with it. I'm looking for the proper way for all four cases dealing with thread termination: a) FreeOnTerminate=False, thread terminates internally (e.g. exception, finished, etc.) b) FreeOnTerminate=True, thread terminates internally c) FreeOnTerminate=False, thread being terminated by calling Terminate d) FreeOnTerminate=True, thread being terminated by calling Terminate No matter what I do, I run into problems. As far as I have read meanwhile, it is common practice for cases b) and d), i.e. when using FreeOnTerminate, to have an OnTerminate handler which sets the thread reference to NIL so that I can be sure to have a valid reference to the thread. But now, in case d) I cannot use Code: [Select]FThread.Terminate; FThread.WaitFor; because the thread might get freed in between. Obviously, pepople work around this problem by setting FThread.FreeOnTerminate:=false before using the "Terminate - WaitFor - Free" cascade. Is this correct? In case c), I get sporadic access violations in the WaitFor instruction. Very bad. So from this point of view, it seems better to enable FreeOnTerminate because this seems to free the thread most reliably. But it causes heavy problems on the other hand because the thread cannot be stopped and freed proberly when closing the program. I get a wild mixture of access violations and memory leaks when trying to do this. The WaitFor instruction on the other hand waits forever if the thread hasn't been started. Very bad because there's actually no need to wait in this case. So what is the actual best practice to terminate a thread? Should FreeOnTerminate be enabled or disabled? How to deal with WaitFor? ---------------------------------------------------------------------- Thaddy · 2024-06-04 16:17 It greatly depends on the process / code you are running in a thread. If your thread is running:Code: Pascal  [Select][+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i repeat//your threaded code, without giving up anything, keeps on running//this sort of programming can't be terminated from outside.//this needs a hard kill.until true=false;type of code, you will actually need to kill the thread, unless in the repeat/until you give up a time slice to the main program. On Windows it is as simple as Sleep(0), not sleep(1), because that is a mistake. Killing a thread that does IO is out of the question: damages files. Look at what is done IN the thread and give it a chance to react to a terminate request. For beginners - and you are a beginner, otherwise you would have known this, Martin Harvey still wrote the Golden Goose of thread programming in Object Pascal: There are several links including on my website but I prefer this one: https://www.seti.net/engineering/threads/threads.php Everything there applies to FreePascal too and all code compiles in mode delphi. Recommended for everyone that has problems with threads. Actually, threads have problems with clumsy programmers, it is the other way around... ---------------------------------------------------------------------- rvk · 2024-06-04 16:24 Quote from: piola on June 04, 2024, 05:55:46 pmSo what is the actual best practice to terminate a thread? Should FreeOnTerminate be enabled or disabled? How to deal with WaitFor? Why do you need the WaitFor? If you need it, you should set FreeOnTerminate to false and handle freeing the thread yourself. If you don't need it, the don't call it and set FreeOnTerminate to true. ---------------------------------------------------------------------- Thaddy · 2024-06-04 16:37 Not true, Rik. Code like a showed never terminates. You have to kill the thread the hard way. And I fear he has just written such code.... Happens too many times on this forum. YOU assume short lived threads, HE assumes long lived threads.... You can't terminate true=false. (That is, not if the loop does not reliquish time slices to the other threads.) If you write infinite threads, you can't kill them without communication. Or order a SIGKILL. Not that infinite threads are evil, think webservers, but when a server needs a restart that involves hard-killing its running threads. Not that OnTerminate/Terminated bogus ---------------------------------------------------------------------- piola · 2024-06-04 16:44 Quote from: rvk on June 04, 2024, 06:24:04 pmWhy do you need the WaitFor? I need WaitFor to terminate the thread properly when the program exits. Otherwise, the thread seems to be killed which (as I have written above) gives a wild mixture of access violations and memory leaks. I don't want that. ---------------------------------------------------------------------- Thaddy · 2024-06-04 16:46 Then you really have to show me what is inside the loop that the thread performs: it is probably close to my example of bad thread programming.. Calling waitfor on such threads makes no sense. ---------------------------------------------------------------------- piola · 2024-06-04 16:48 Quote from: Thaddy on June 04, 2024, 06:37:08 pmHE assumes long lived threads Yes, it's a http client which performs http queries from time to time. We can assume that the thread behaves well and checks the Terminated property from time to time. It's not a "block until true equals false" thread. But depending on the situation, it might not terminate immediately either. It might take up to 60 seconds if it's waiting for an I/O timeout; this might happen. If the thread is idle, it might equally well terminate immediately when calling Terminate. That's why I wrote I want to have a best practice which just works. ---------------------------------------------------------------------- rvk · 2024-06-04 16:56 Quote from: Thaddy on June 04, 2024, 06:37:08 pmNot true, Rik. Code like a showed never terminates. You have to kill the thread the hard way.????????? How do you know? Where do you see his code in the thread? Am I missing something. Maybe he has a loop with while not terminated. Are you assuming things or am I missing something? The only code in start post is Code: Pascal  [Select][+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i FThread.Terminate;FThread.WaitFor;Which would work just fine if you have a loop like while not terminated. ---------------------------------------------------------------------- piola · 2024-06-04 16:59 I can show the Execute procedure, it's not secret: Code: [Select]    try       if not Init then Exit;       while not Terminated do begin         if (FEvent.WaitFor (1000) = wrSignaled) and not Terminated then begin           task := PopNextTask;           if task.Valid then ProcessTask (task);         end;       end;       Disconnect;     finally       HandleAbort;     end; But the code of the other methods is longer. Basically, the http request is done within ProcessTask. Before questions arise: The HandleAbort calls an OnFatalError if one is assigned. This handler is cleared in the call to Terminate. ---------------------------------------------------------------------- Thaddy · 2024-06-04 17:30 Ask you rightly suggested, the issue is probably in processtask. Does that sets terminated? any where? Can processtask be aborted itself? ---------------------------------------------------------------------- alpine · 2024-06-04 17:43 Quote from: piola on June 04, 2024, 05:55:46 pmIn case c), I get sporadic access violations in the WaitFor instruction. Very bad. I'm bit surprised by that. Do you have also an OnTerminate handler which accidentally frees the thread instance? Quote from: piola on June 04, 2024, 05:55:46 pmSo from this point of view, it seems better to enable FreeOnTerminate because this seems to free the thread most reliably. But it causes heavy problems on the other hand because the thread cannot be stopped and freed proberly when closing the program. I get a wild mixture of access violations and memory leaks when trying to do this. IMHO using FreeOnTerminate is always a bad idea, See https://forum.laz arus.freepascal.org/index.php/topic,62182.msg470478.html#msg470478 Quote from: piola on June 04, 2024, 05:55:46 pmThe WaitFor instruction on the other hand waits forever if the thread hasn't been started. Very bad because there's actually no need to wait in this case. It shouldn't be the case, since WaitFor depends only on a valid thread handle, which is created by the TThread.Create. Anyway, you can just make sure you have started the thread before calling the WaitFor. Quote from: piola on June 04, 2024, 05:55:46 pmSo what is the actual best practice to terminate a thread? Should FreeOnTerminate be enabled or disabled? How to deal with WaitFor? Mine is not to use FreeOnTerminate, monitor the TThread.Terminated closely and then shutdown in graceful way. Freeing is strictly into the caller thread. Surely there is a way to shorten the I/O timeouts. PS: AFAIR there was an issue with Linux where the finished threads are left in an 'join-able' state after finishing and not calling WaitFor will leak resources, so it is almost mandatory to call it. ---------------------------------------------------------------------- rvk · 2024-06-04 18:45 And handle exceptions correctly within the thread. Don't let it crap out. Otherwise investigate the use of TThread.FatalException carefully. ---------------------------------------------------------------------- cdbc · 2024-06-04 22:52 Hi QuotePS: AFAIR there was an issue with Linux where the finished threads are left in an 'join-able' state after finishing and not calling WaitFor will leak resources, so it is almost mandatory to call it.Yes @alpine, according to /posix/ you have to 'join' a thread when it finishes, for the OS to reclaim resources. This is done by one of two methods:  1) calling "WaitFor"(on a thread handle), this is what *TThread.WaitFor* does   2) calling "EndThread" as the /last/ command/line in in your ThreadFunc,      this is what "FreeOnTerminate" does, just before it free's itself. There's another possibility: Set "FreeOnTerminate" to false. Override the "TObject.BeforeDestruction" method(in your thread) and in that method you check if the thread is assigned & running, if so, call "Terminate" and "Waitfor" before the "inherited BeforeDestruction". When you do this, you can just call 'YourThread.Free' and it will handle it itself, taking its time to waitfor and all... This method works very well, *when* you check for "Terminated" in the thread code/loop. HTH Regards Benny edit: small additions ---------------------------------------------------------------------- alpine · 2024-06-05 06:39 Quote from: cdbc on June 05, 2024, 12:52:28 amHi QuotePS: AFAIR there was an issue with Linux where the finished threads are left in an 'join-able' state after finishing and not calling WaitFor will leak resources, so it is almost mandatory to call it.Yes Remy, according to /posix/ you have to 'join' a thread when it finishes, for the OS to reclaim resources. I'm just noting, once I have investigated it for another thread here in the forum. It is not an issue with me since I always follow the same pattern when using threads, which doesn't reveal it. ---------------------------------------------------------------------- cdbc · 2024-06-05 06:43 Hi Arhhh, f*ck me. I'm sorry mate, Alpine I'll correct it immediately. Regards Benny ---------------------------------------------------------------------- alpine · 2024-06-05 07:06 Quote from: cdbc on June 05, 2024, 08:43:47 amHi Arhhh, f*ck me. I'm sorry mate, Alpine I'll correct it immediately. Regards Benny NPR, I guess it is because of the red glow in Dave's face ---------------------------------------------------------------------- cdbc · 2024-06-05 08:09 Hi I think you've got a point there  Regards Benny ---------------------------------------------------------------------- piola · 2024-06-05 09:01 Quote from: Thaddy on June 04, 2024, 07:30:10 pmAsk you rightly suggested, the issue is probably in processtask. Does that sets terminated? No. Why should it? It's the main thread which calls Terminate when the program is about to exit. QuoteCan processtask be aborted itself? Yes. In case of a connection error, for example. ---------------------------------------------------------------------- rvk · 2024-06-05 09:18 Quote from: piola on June 04, 2024, 05:55:46 pmIn case c), I get sporadic access violations in the WaitFor instruction. Very bad. So from this point of view, it seems better to enable FreeOnTerminate because this seems to free the thread most reliably.No, in that case it's not better to use FreeOnTerminate. It's better to find out where the access violation comes from. TThread.Terminate only sets a Terminated flag. The only thing I can think of is that the access violation comes from the loop ending the Execute. (but an AV from the thread shouldn't slip through to the main thread, so it depends WHERE that AV occurs.) But because TThread.WaitFor gives you an error, that doesn't mean you can ignore it and search for other solutions. You need to understand why it gives you that error. ---------------------------------------------------------------------- piola · 2024-06-05 09:42 Quote from: alpine on June 04, 2024, 07:43:56 pmQuote from: piola on June 04, 2024, 05:55:46 pmIn case c), I get sporadic access violations in the WaitFor instruction. Very bad. I'm bit surprised by that. Do you have also an OnTerminate handler which accidentally frees the thread instance? Yes, but it's removed before termination: Code: [Select]      FComm.OnTerminate := nil;       FComm.Terminate;       FComm.WaitFor;       FreeAndNil (FComm); ---------------------------------------------------------------------- piola · 2024-06-05 10:01 I've had just another idea: Is it ok to set FreeOnTerminate:=false and free the thread within the OnTerminate event? ---------------------------------------------------------------------- alpine · 2024-06-05 10:26 Quote from: piola on June 05, 2024, 11:42:29 amQuote from: alpine on June 04, 2024, 07:43:56 pmQuote from: piola on June 04, 2024, 05:55:46 pmIn case c), I get sporadic access violations in the WaitFor instruction. Very bad. I'm bit surprised by that. Do you have also an OnTerminate handler which accidentally frees the thread instance? Yes, but it's removed before termination: Code: [Select]      FComm.OnTerminate := nil;       FComm.Terminate;       FComm.WaitFor;       FreeAndNil (FComm); Are you sure that yours FComm.Terminate; actually is the reason for the termination? And the FComm reference isn't dangling? Did you read the link I gave you in reply #10? What about the advice from rvk in reply #11? ---------------------------------------------------------------------- rvk · 2024-06-05 10:29 Quote from: piola on June 05, 2024, 12:01:44 pmI've had just another idea: Is it ok to set FreeOnTerminate:=false and free the thread within the OnTerminate event? No, you can't free anything in its own event. You can post a CM_RELEASE and handle it in a message handler. As seen here https://stackoverflow.com/a/2502613/1037511 But that wouldn't solve your problems. The WaitFor technique should just work but there is something else wrong. ---------------------------------------------------------------------- cdbc · 2024-06-05 10:31 Hi QuoteI've had just another idea: Is it ok to set FreeOnTerminate:=false and free the thread within the OnTerminate event?Methinks you're trying to treat the symptoms, *not* the disease, That's not always a good idea. Look for the reason... Regards Benny ---------------------------------------------------------------------- piola · 2024-06-05 11:10 Quote from: alpine on June 05, 2024, 12:26:20 pmAre you sure that yours FComm.Terminate; actually is the reason for the termination? And the FComm reference isn't dangling? No. It should, but I'm not 100% sure. Code: [Select](1) FComm.FreeOnTerminate := false; (2) FComm.OnTerminate := nil; (3) FComm.Terminate; (4) FComm.WaitFor; (5) FreeAndNil (FComm); I am sure in line 1. I thought I can rely on this block not being interrupted, but now I have seen that WaitFor internally might call CheckSynchronize. So do I have to expect that this block might be interrupted? QuoteDid you read the link I gave you in reply #10? Yes, and I knew it before. It was part of my "Google research" Quote] What about the advice from rvk in reply #11? There's an exception handler within ProcessTask which catches all exceptions and terminates the thread in this case. ---------------------------------------------------------------------- piola · 2024-06-05 11:22 Quote from: rvk on June 05, 2024, 12:29:41 pmQuote from: piola on June 05, 2024, 12:01:44 pmI've had just another idea: Is it ok to set FreeOnTerminate:=false and free the thread within the OnTerminate event? No, you can't free anything in its own event. Pity, because it worked flawlessly. On the other hand, if enable FreeOnTerminate in the OnTerminate event handler, I get a memory leak. Which irritates me because the topic mentioned afore says that the OnTerminate handler is called first, then FreeOnTerminate is checked. So it should be possible to activate FreeOnTerminate in the event handler, shouldn't it? QuoteThe WaitFor technique should just work but there is something else wrong. I haven't overlooked that, but it still can't figure out what might go wrong. That's why I'd like to have a best-practice-advice first. I have followed the advice now not to use FreeOnTerminate. That's ok so far. But I have two situations: a) The program might want to disconnect from the server, so the http communication thread should terminate. In this case, I don't want to use WaitFor, but just let the thread finish in the background. But I have to free the thread object somewhen later. That's why I asked whether I can do that in the OnTerminate. b) The program is about to exit. Then I *must* wait for the thread to have it terminate gracefully. ---------------------------------------------------------------------- alpine · 2024-06-05 11:51 My advice: Don't risk race conditions by using FreeOnTerminate and/or OnTerminate. In your case you can simply define a boolean property, say 'Completed' and set it finally to True in your Execute method. Then check it regularly or when you're about to allocate a new thread, and if True then do a WaitFor and Free. You must check it also at exit of your program, of course. Then you should call Terminate if it is not 'Completed'. ---------------------------------------------------------------------- piola · 2024-06-05 12:28 Quote from: alpine on June 05, 2024, 01:51:56 pmMy advice: Don't risk race conditions by using FreeOnTerminate and/or OnTerminate. In your case you can simply define a boolean property, say 'Completed' and set it finally to True in your Execute method. Then check it regularly or when you're about to allocate a new thread, and if True then do a WaitFor and Free. You must check it also at exit of your program, of course. Then you should call Terminate if it is not 'Completed'. This is a good idea. Could I use the existing Finished property as well instead of introducing yet another one? ---------------------------------------------------------------------- alpine · 2024-06-05 12:56 Quote from: piola on June 05, 2024, 02:28:26 pmQuote from: alpine on June 05, 2024, 01:51:56 pmMy advice: Don't risk race conditions by using FreeOnTerminate and/or OnTerminate. In your case you can simply define a boolean property, say 'Completed' and set it finally to True in your Execute method. Then check it regularly or when you're about to allocate a new thread, and if True then do a WaitFor and Free. You must check it also at exit of your program, of course. Then you should call Terminate if it is not 'Completed'. This is a good idea. Could I use the existing Finished property as well instead of introducing yet another one? Nope! It doesn't mean that it is actually finished. It is just something your Execute method should check to know that it is instructed to shutdown. Edit: Not true. ---------------------------------------------------------------------- piola · 2024-06-05 13:07 Just to make sure we're talking about the same thing: I'm speaking about TThread.Finished, not TThread.Terminated. ---------------------------------------------------------------------- alpine · 2024-06-05 13:13 Quote from: piola on June 05, 2024, 03:07:17 pmJust to make sure we're talking about the same thing: I'm speaking about TThread.Finished, not TThread.Terminated. You're right. Finished is just what I've meant by 'Completed'. It goes True just before FreeOnTerminate check in ThreadFunc. No need to introduce anything new. Sorry, my bad. ---------------------------------------------------------------------- rvk · 2024-06-06 09:26 BTW. What's wrong with using TThread.WaitFor on a terminated, but net yet freed TThread ?? This works for me: Code: Pascal  [Select][+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i type  TMyThread = class(TThread)  private  protected    procedure Execute; override;  end; procedure TMyThread.Execute;beginend; //...var  T: TMyThread;begin  T := TMyThread.Create(false);  Sleep(2000);  T.Terminate;  T.WaitFor; // even though there is no Execute (or has finished) WaitFor should still work but passthroughend; So the original problem, where WaitFor is a problem when the thread has already terminated, but not freed, lies somewhere else. ---------------------------------------------------------------------- alpine · 2024-06-06 11:41 Quote from: rvk on June 06, 2024, 11:26:15 amBTW. What's wrong with using TThread.WaitFor on a terminated, but net yet freed TThread ?? Nothing wrong! Otherwise it will be almost useless ... As a matter of fact it works from the very moment of TThread.Create until finally freed. ---------------------------------------------------------------------- Thaddy · 2024-06-06 13:28 Indeed.