Threads - stringlist and programming tips

[cris75]

Being not that much familiar in thread programming I often say to myself to think carefully to what I do and I am also often insecure too, I just came across the need to use a simple StringList in my TThread derived class, I was going do it "in the usual way" inside the execute method (with try/except/finally of course), but then I asked myself what could happen if the thread terminates earlier than expected for whatever reason..
That way, supposing that in the try/finally block an exception is not raised, the StringList would still not be freed and that would be a leak (bug), right?
Is it ok then to declare the StringList private in my thread class and free it in a thread destructor?
Am I right to think that these precautions are necessary and, in principle, I have always to think carefully to what could happen when a thread terminates when this isn't supposed to happen?
Thank you,
Cris

[alpine]

Being not that much familiar in thread programming I often say to myself to think carefully to what I do and I am also often insecure too, I just came across the need to use a simple StringList in my TThread derived class, I was going do it "in the usual way" inside the execute method (with try/except/finally of course), but then I asked myself what could happen if the thread terminates earlier than expected for whatever reason..
That way, supposing that in the try/finally block an exception is not raised, the StringList would still not be freed and that would be a leak (bug), right?
Is it ok then to declare the StringList private in my thread class and free it in a thread destructor?

Sure It is.

As that string list is a private one, you can use the RAII:

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

1. procedure TMyThread.Execute;
2. begin
3.   list := TStringList.Create;
4.   try
5.  
6.     while not Terminated do
7.       ; //...
8.  
9.   finally
10.     list.Free;
11.   end;
12. end.

Am I right to think that these precautions are necessary and, in principle, I have always to think carefully to what could happen when a thread terminates when this isn't supposed to happen?

What is your concern? How do you suspect to terminate besides the "usual" way, i.e. TThread.Terminate? Exception?

[PascalDragon]

Being not that much familiar in thread programming I often say to myself to think carefully to what I do and I am also often insecure too, I just came across the need to use a simple StringList in my TThread derived class, I was going do it "in the usual way" inside the execute method (with try/except/finally of course), but then I asked myself what could happen if the thread terminates earlier than expected for whatever reason..
That way, supposing that in the try/finally block an exception is not raised, the StringList would still not be freed and that would be a leak (bug), right?

The only way a thread terminates in a way that isn't cooperatively is by someone using a thread-kill API and that should be avoided anyway, cause the thread might hold a lock or something and killing it would leave that in a dangerous state. So simply don't do something like that and use resource protection blocks inside the Execute method. Putting a variable in the class as a field instead of using a local variable is mainly useful if you want to split it's usage into separate methods of the TThread descendant (but nothing's stopping you of course to do it anyway and it would be right as well).

[Warfley]

Do you intend to kill the thread forcefully as part of your program, or are you just thinking of this maybe happening externally. If you don't want to forcefully kill the thread yourself, I think it is not so likely that this will happen externally, to warrant security precautions. Because if this happens uncontrolled, you may have much more problems, like PascalDragon wrote there may be other resources like locks acquired.
Also if you use a String list I assume you handle strings, which are also dynamic memory where the compiler just transparently does the create and free in the background. So when a thread forcefully terminates, it's not just your StringList which is going to leak. So best to not overthink that.

Also a memory leak in such incredibly rare cases is not the end of the world. When the OS or User starts to randomly forcekill threads, the rest of the application is probably also not going to run as expected and will probably soon be killed anyway.

It's of course a different story if you intend to forcekill your thread as part of your program flow, which is going to make this alot more complicated

[alpine]

The way one thread must terminate is when its Execute method reached its end.
For that reason the Terminate method and Terminated property were provided. The Execute method must provide an exit path in case the Terminated becomes True with all required cleanup. That is the right thing to do.

Using thread API to forcefully terminate thread is considered very dangerous for the reasons PascalDragon noted. More details here: https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-terminatethread#remarks

If, for some reason, the OS decided to terminate a thread, it will work on a process level and will release all its threads, resources, etc. You don't have to worry about leaks as it will all disappear.

In this regard, use the following rules of thumb:

• Enclose the whole Execute body with a try/except block with appropriate handling and make sure the exception doesn't escape the thread.
• Enclose all your Create/Free, Enter/Leave, Lock/Unlock pairs into a try/finally blocks.
• Check the Terminated property as often as you can in order to quit processing as soon as it becomes True.
• If it is a one-shot thread, prefer to prepare its input data beforehand and to use its output data after it was successfully joined with TThread.WaitFor.
• If it is a continuous background thread and you want to visualize the output from it, prefer to use a thread-safe queue and/or Application.QueueAsyncCall, transferring it the data ownership instead of TThread.Synchronize. The latter is a real performance bottleneck.
• Design for a light threads that can be easily re-spawned when crashed.

Assuming you have enclosed the Execute body with a try/except block then you can use a silent exception to quickly realize the exit path from the nested calls, e.g.

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

1.   if Terminated then Abort;

 

[Warfley]

The way one thread must terminate is when its Execute method reached its end.

Well it should terminate that way. There may be situations in which you want to force kill a thread. E.g. I have once worked with a library which performed a blocking call with no way to set a timeout or interrupt it in any friendly way. This library performed actions on a file in a network drive, and when the network connection was bad it simply would get stuck with no way terminate except through a force kill.

My solution back then was to send a signal to the thread (Unix only), which would do as much cleanup as possible before forcekilling the thread. This was "good enough" for the time (there were still some minor leaks but as this happens so rarely, not a real problem). Another solution I experimented later with was to raise a custom exception, which would trigger all the finally blocks, and being cought on the lowest level which would then trigger a forcekill. This allows to make use of local variables with try-finally, rather then doing cleanup in a separate function with only access to global/class variables. But I never got more than a very simple proof of concept for this idea.

But that's what I meant in my post, when you need to forcekill as part of your control flow, Things get complicated. It's not impossible to utilize force kills if you design your thread code accordingly, but it is definitely not pretty, and requires a deep understanding of everything that's going on in the thread

[PascalDragon]

• Enclose the whole Execute body with a try/except block with appropriate handling and make sure the exception doesn't escape the thread.

This is not necessary if you don't need to handle it in the thread, because the call to the Execute body in TThread is already protected by a try … except-block and any raised exception will be captured in TThread.FatalException.

[Thaddy]

Terminate and Terminated are a function resp. a flag. The most important thing with threads is probably waitfor which many of you do not have the patience for.

[alpine]

Terminate and Terminated are a function resp. a flag. The most important thing with threads is probably waitfor which many of you do not have the patience for.

Right. I corrected my post, "joined" --> waitfor.

[Warfley]

Terminate and Terminated are a function resp. a flag. The most important thing with threads is probably waitfor which many of you do not have the patience for.

Don't use WaitFor in an interactive (e.g. GUI) application, because it will block the main thread. Use OnTerminate or FreeOnTerminate instead.

The main problem with waiting after calling terminate is that it can take arbitrary long until the thread finishes, as terminate is just a "hint" that you would like for the thread to somewhen in the future, when it has time to do so, stop prematurely.
If you are in an HTTP request with a timeout of 5 seconds, your terminate will not have any effect for 5 seconds. I've worked with HTTP based protocols where the http request would trigger a User input before returning, where the timeout is in the order of minutes.

So when you call WaitFor after Terminate you may freeze your main thread for a few seconds up to minutes or even indefinitely, depending on what your thread is currently doing.
Also I would argue that there is generally speaking very rarely the need to use WaitFor at all. If you don't need the result of a thread, the best you can do is setting FreeOnTerminate then call terminate and just "throw the thread away". It will finish all on its own and when it does so it will free itself, meaning you don't have to care about it anymore.
If you need partial results, use the OnTerminate event, to react to the thread finishing without waiting for it.

Only use WaitFor is a few cases, first If you have a batch application, which has nothing better to do than waiting, second when you are in the cleanup of your program (i.e. the user wants to close) and your UI is already being destroyed/you don't get any new events in that you are blocking by WaitFor. But in any normal interactive program flow, do not use WaitFor on the main thread at any time

[alpine]

Terminate and Terminated are a function resp. a flag. The most important thing with threads is probably waitfor which many of you do not have the patience for.

Don't use WaitFor in an interactive (e.g. GUI) application, because it will block the main thread. Use OnTerminate or FreeOnTerminate instead.

*snip*

Only use WaitFor is a few cases, first If you have a batch application, which has nothing better to do than waiting, second when you are in the cleanup of your program (i.e. the user wants to close) and your UI is already being destroyed/you don't get any new events in that you are blocking by WaitFor. But in any normal interactive program flow, do not use WaitFor on the main thread at any time

OnTerminate can be used to postpone the WaitFor and I'll prefer to always use WaitFor instead of FreeOnTerminate=True. With the latter we trudge back to things like dangling refs, double free, PSA: Don't use FreeAndNil, etc.

No, I won't take that bait again 

[Warfley]

I think FreeOnTerminate is usually the cleanest solution, but as soon as you set it to true, you are not allowed to touch the TThread object outside it's own context (I've. In its own execution, or in the main thread during an event that was fired by the thread that is synchronized).

I have no idea what you mean with postponing WaitFor, if you call WaitFor you freeze your current thread (and If it's your main thread, your whole application) until the thread is finished. OnTerminate allows you to have code be executed when the thread is terminated without having to wait for it, and thereby not freezing up the application.

The main "problem" with OnTerminate is the inconvenience of having to splice up your code between different methods, which is not pretty but workable. WaitFor should only ever be used (outside the exceptions I have written above), when you are really Shure that the thread will be stopped within a few milliseconds after Terminate was called. I.e. when your thread is not currently doing a blocking I/O or doing heavy computations, where you can't regularly check the terminated flag (i.e. the two things you most likely want to use a thread for).

So especially for those that are new to threading (as the thread creator seems to be), as a rule of thumb, stick away from WaitFor until you know really good what you are doing, otherwise you may freeze your application.

[alpine]

So especially for those that are new to threading, as a rule of thumb, don't set FreeOnTerminate to true. Otherwise, every time you use the thread reference, there's no way to know if it's a dangling reference. Even to tell it to terminate, i.e. thread.Terminate

[Warfley]

Well I would probably even go a step further, in that, no matter if you are a beginner or an advanced user, you should probably stay away from threads as much as possible if you can. Threading is probably the single most complex issue programmers can face and it's extremely hard to get right.


It's not just WaitFor and FreeOnTerminate, those are just too examples of the same underlying problem. If you have no Synchronisation at all you run into the FreeOnTerminate problem, where you can't know if the data is still valid at a certain point in time. But if you do synchronization ( like with WaitFor, but also synchronize, critical sections or events), you can freeze up your whole application or force serialize your threads that you could also just not use them at all.

My general recommendation is therefore still that best to not use any threads if not necessary.

As an example, I am currently writing a program that simply downloads a large ZIP archive (lazarus sources) and unzips them via TZipper. I put it in a thread with an Progressbar update event. But here is the catch, how can I stop this thread prematurely? Neither TFPHTTPClient nor TZipper have a function to stop mid process, so once one them started, it's around 5-10 minutes of the thread not being able to terminate. So any waitfor would Freeze the application for 5-10 minutes, resulting in the App being killed by windows.

I have a solution, but I know that you probably don't like it (raising an exception in the Progress event that will stop the execution of the thread). But if I would follow the advice here, only using Terminate and WaitFor afterwards, my application would not be usable.

[balazsszekely]

@Warfley

As an example, I am currently writing a program that simply downloads a large ZIP archive (lazarus sources) and unzips them via TZipper. I put it in a thread with an Progressbar update event. But here is the catch, how can I stop this thread prematurely? Neither TFPHTTPClient nor TZipper have a function to stop mid process, so once one them started, it's around 5-10 minutes of the thread not being able to terminate. So any waitfor would Freeze the application for 5-10 minutes, resulting in the App being killed by windows.

Both TFPHTTPClient/T(Un)Zipper has a terminate method to immediately stop the download/zipping process.