Proper way to terminate a thread?

[alpine]

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

Hi
I think you've got a point there 
Regards Benny

[piola]

Ask 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.

Can processtask be aborted itself?

Yes. In case of a connection error, for example.

[rvk]

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.

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]

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?

Yes, but it's removed before termination:

Code: [Select]

      FComm.OnTerminate := nil;
      FComm.Terminate;
      FComm.WaitFor;
      FreeAndNil (FComm);


[piola]

I've had just another idea: Is it ok to set FreeOnTerminate:=false and free the thread within the OnTerminate event?

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

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]

I'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]

Hi

I'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]

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

Did you read the link I gave you in reply #10?

Yes, and I knew it before. It was part of my "Google research"

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

I'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?

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

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]

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'.

This is a good idea. Could I use the existing Finished property as well instead of introducing yet another one?

[alpine]

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'.

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]

Just to make sure we're talking about the same thing: I'm speaking about TThread.Finished, not TThread.Terminated.