Proper way to terminate a thread?

[piola]

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]

It greatly depends on the process / code you are running in a thread.
If your thread is running:

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

1. repeat
2. //your threaded code, without giving up anything, keeps on running
3. //this sort of programming can't be terminated from outside.
4. //this needs a hard kill.
5. 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]

So 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]

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]

Why 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]

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]

HE 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]

Not 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][+][-]

1. FThread.Terminate;
2. FThread.WaitFor;

Which would work just fine if you have a loop like while not terminated.

[piola]

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]

Ask you rightly suggested, the issue is probably in processtask. Does that sets terminated? any where? Can processtask be aborted itself?

[alpine]

In 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?

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.

IMHO using FreeOnTerminate is always a bad idea, See https://forum.lazarus.freepascal.org/index.php/topic,62182.msg470478.html#msg470478

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.

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.

So 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]

And handle exceptions correctly within the thread.
Don't let it crap out.
Otherwise investigate the use of TThread.FatalException carefully.

[cdbc]

Hi

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.

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]

Hi

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.

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]

Hi
Arhhh, f*ck me.
I'm sorry mate, Alpine I'll correct it immediately.
Regards Benny