Is this a correct way of using OmniThreadLibrary - terminate the existing one then create a new one?

Question

I use the excellent OmniThreadLibrary library to implement threaded source code parsing, the program need to abandon the existing parsing and restart the parsing whenever the source code is changed.

I do this with the code snippet shown below, is it the correct way? Do I still have to check the Terminated property of the thread in the ThreadedParseHtml function?

if FParserThread <> nil then
begin
  FParserThread.RemoveMonitor;
  FParserThread.Terminate(500);
end;

FParserThread := CreateTask(ThreadedParse);
FParserThread.SetParameter('SourceCode', Editor.Lines.Text);
FParserThread.MonitorWith(FParserThreadMonitor);
FParserThread.Run;

Thanks in advance!

Edit 1: Sorry for reopening this question, but I found memory leaks when FParserThread is not completed by itself by calling the Terminate method with enough time given... Any ideas as to what might cause the memory leaks? Thanks!

Edit 2: Read this blog post, I still couldn't figure what the problem might be, since after every steps in ThreadedParse the code break if Terminated is ture...

Edit 3: Answering Rob's questions:

1. In the OnTerminated event handler (not shown here), FParserThread is set to "nil", so by "FParser is completed by itself", I mean the if FParserThread <> nil then block is not executed, in that case FParserThread is terminated because it's parsing has been completed.

2. The logic behind the code is that, this is a code editor, upon any code edits there will be a thread being started to parse the source code into the internal tree presentation, in the case when a new code edit happens but the previous parsing thrad hasn't been edited, the program will first forcibly the previous parsing thread then start a new one. This maybe is not a good approach...

Edit 4: After reading this similar SO question, I changed my code to call FParserThread.Terminate without a parameter which means, if I understand it correctly, that statement will only signal the thread to end, and inside the actual thread task, I applied the logic to exit the thread execution if the Terminated property is True.

Now what's wired is that, with the help of Tracetool, I found that after calling FParserThread.Terminate the OnTaskMessage event (where I clean up the memories) would not be fired again, that's what caused the memory leaks....

Answer 1

You don't have to check the Terminated property in the associated task. You're calling Terminate(1), which will forcefully kill the thread if it doesn't end itself within the 1 ms window that you've specified.

However, it's really not a good idea to forcefully kill a thread. That thread might have possessed a mutex or critical section when you killed it, so killing it will leave shared data in an inconsistent state. That could have a detrimental effect on your entire program.

Better is to notify your thread that you want it to terminate, but give it a more realistic deadline for termination. Within the other thread, you should occasionally check whether the thread has been asked to terminate, and then have it terminate itself gracefully.

If the thread doesn't end within the specified time limit, then you have bigger problems, and forcefully killing it won't solve them.

Answer 2

OP here, after using OmniThreadLibrary for over 2 years now, my conclusion is that the proper way of stopping a OTL task is using the cancellation tokens. Code Example below.

In the caller thread (usually the main thread), call:

//this will tell (not kill) the thread identified by myTask to stop.
myTask.CancellationToken.Signal;

In the callee thread you must regularly check the task.CancellationToken.IsSignaled property, if it becomes true, exit the execution and the thread termination will be handled by the system and OTL:

if task.CancellationToken.IsSignaled then
  Exit;