1

I have found this nice component called TBackgroundWorker. However, people are criticizing it (on SO) because it uses TerminateThread. Here is the "faulty" code:

destructor TBackgroundWorker.Destroy;
begin
  if IsWorking then
  begin
    TerminateThread(fThread.Handle, 0);
    Cleanup(True);
    raise EBackgroundWorker.CreateFmt(SInvalidExit, [Name]);
  end;
  inherited Destroy;
end;

For me it seems a valid destructor. Is it? Should I worry?
There is a better solution?

Gabriel
  • 20,797
  • 27
  • 159
  • 293
  • Where specifically at SO are *people* criticizing it? Why are they saying that TerminateThread is an issue? It also only calls `TerminateThread` if it is being closed inappropriately (look at the definition of `SInvalidExist`). – Ken White Feb 05 '19 at 13:07
  • Here: https://stackoverflow.com/questions/21430812/background-worker-delphi#comment59234135_21431160 but it is not the only place. – Gabriel Feb 05 '19 at 13:10
  • See my edited comment. You've linked a single comment which admits that it's based on a quick review, and that a more thorough review of the code would be needed. It also appears that you need to do a more thorough review of the code. :-) (My suggestion to look at the definition should say `SInvalidExit`, not `SInvalidExist`, of course. It's at approximately line 97 or so.) – Ken White Feb 05 '19 at 13:11
  • 5
    I think Raymond Chen is a trusted authority on telling people not to use TerminateThread... – Stefan Glienke Feb 05 '19 at 13:25
  • What does the documentation say? – David Heffernan Feb 05 '19 at 13:29
  • @StefanGlienke - I think you refer to this? "But please, stop calling Terminate­Thread. There are no valid use cases for it. Any time you call it, you will corrupt the target process, so you may as well just terminate the entire process and be done with it" – Gabriel Feb 05 '19 at 13:50
  • I am referring to the entire blog article that was linked in the comment you mentioned – Stefan Glienke Feb 05 '19 at 14:00
  • @DavidHeffernan - Hi. The [documentation](https://blogs.msdn.microsoft.com/oldnewthing/20150814-00/?p=91811) says it is NOT ok. Question answered. Case close :) Many thanks guys. – Gabriel Feb 05 '19 at 14:04
  • 3
    Also, above code (regardless of TerminateThread issue) is not valid Delphi destructor. Delphi destructors should never ever raise an exception, unless you plan to kill the whole app at that point. – Dalija Prasnikar Feb 05 '19 at 14:55
  • 2
    Looks like an emergency exit, probably you're not expected to free the component while the worker is working. – Sertac Akyuz Feb 05 '19 at 16:57
  • 1
    @SertacAkyuz: Yep. That's what I saw also. *It only calls TerminateThread if it is being closed inappropriately (look at the definition of SInvalidExit.* (*inappropriately* should be *improperly*.) – Ken White Feb 05 '19 at 21:10
  • @SertacAkyuz It may be emergency exit, but if you don't spell it out, someone is going to use in other situations, too. Or will use destructor that can raise exception because in this example TerminateThread was _the only bad code_ and the rest is fine. – Dalija Prasnikar Feb 05 '19 at 22:31
  • But still. Raymond Chen says: "There are NO valid use cases for it". If this is true (and I do put all my trust in that great guy) how to close gracefully. – Gabriel Feb 06 '19 at 08:36
  • When you need to terminate a thread that you don't control, then you use TerminateThread. – Sertac Akyuz Feb 07 '19 at 17:17

2 Answers2

1

In my opinion, the destructor is valid.

Forcibly terminating a thread is wrong. Also, raising an exception in a destructor may kill the whole application. However, please don't ignore the context.

We talk about a proxy object that wraps a thread. If such a component is running, its destruction is comparable to killing a running thread. The proxy should fail fast and report such a misaction, not manipulate it. Besides, this is a third-party component, which does not know the intent of the application's developer.

I suppose you disagree with me; otherwise, we didn't have this conversation. Let's see what the alternatives are.

  1. Canceling the task and terminating the thread gracefully, no exception message. With this approach, we are guessing the intention of the developer. If the developer has made a mistake, he or she may never know until it is too late. The application would have unexpected behaviors, and it is very complicated to figure out the source of the issue.

  2. Ignoring the running thread and destroying the component anyway, without raising an exception. Seems like turning a deterministic machine into a non-deterministic one. Do we even need to discuss this?

  3. Just raising an exception. Because the thread is still running, the variables and stack trace may hold misleading states, which makes debugging much more difficult.

I believe we all like to discover the bugs in the early stage of development and offer a reliable and stable application to our customers. Should we stop doing that because there is no valid use case for the tool we need to use?

There is always a valid use case for something. If I am wrong, please enlight me.

Kambiz
  • 26
  • 3
  • @MartynA, thanks for bringing my mistake into my attention. I edited my answer accordingly. – Kambiz Feb 11 '19 at 12:03
  • Hi Kambiz. I think your component is great. I really APPRECIATE that you made it freeware for Delphi community. I don't disagree with your way of terminating the thread, neither agree. This is why I started the discussion. To see if others can conciliate Raymond Chen's "There are NO valid use cases for it" with "but we still need to end the thread, somehow" :) :) – Gabriel Feb 11 '19 at 14:41
0

For me it seems a valid destructor. Is it? Should I worry?

It is a bad destructor code.

First, everything bad you have heard about TerminateThread is true. There is no safe way to terminate thread as it may leave the application in unstable state, and you should never use that function unless you want to immediately close the application, too. And in such cases it is better to just exit the process altogether. See: Calling TerminateThread on a Windows thread when app exits

Windows started picking up the really big pieces of TerminateThread garbage on the sidewalk, but it’s still garbage on the sidewalk

Now the history.

Originally, there was no Terminate­Thread function. The original designers felt strongly that no such function should exist because there was no safe way to terminate a thread, and there’s no point having a function that cannot be called safely. But people screamed that they needed the Terminate­Thread function, even though it wasn’t safe, so the operating system designers caved and added the function because people demanded it. Of course, those people who insisted that they needed Terminate­Thread now regret having been given it.

It’s one of those “Be careful what you wish for” things.

Additionally, destructor raises an exception, which is something Delphi destructors should never ever do. Raising exceptions in destructor (which are not caught and handled within try..except block) will cause irreparable memory leaks in application.

There is a better solution?

Yes. Since, Cleanup method will call fThread.Free which will wait for thread completion and will perform normal thread shutdown there is no need to call TerminateThread.

Instead of forcing thread termination, it would be better to Cancel the thread and give it time to gracefully terminate itself. This also may require calling WaitFor although pumping Windows messages at that point could interfere with other application code.

destructor TBackgroundWorker.Destroy;
begin
  if IsWorking then
    begin
      Cancel;
      // WaitFor; 
      Cleanup(True);
    end;
  inherited Destroy;
end;

Ultimately, it is not in the domain of component to handle what happens if the thread is still running during shutdown. If there is a need to handle such scenario and prevent shutdown, then this needs to be handled from outside code.

Generally, I would avoid using this component as generalized solutions can create more problems than they are worth. Waiting for a thread by pumping messages is not the greatest design. It may work well in some circumstances and not in others.

It would be better architecture to rely on TThread.WaitFor function. However, TThread.WaitFor is blocking call, so that behavior may not fit well into TBackgroundWorker architecture and desired behavior.

Note: I didn't fully inspect the code of TBackgroundWorker component so there may be other issues that are not covered in this post.

Dalija Prasnikar
  • 27,212
  • 44
  • 82
  • 159