WorkerThread.Dispose might access an already released FWorkEvent

[modersohn]

If TWorkerThread.Dispose is called with CanBlock = False (the way it is currently used), it could access an already released FWorkEvent:

class procedure TWorkerThread.Dispose(CanBlock: Boolean);
var
  LRef: TThread;
begin
  WorkerThread.FreeOnTerminate := not CanBlock;
  WorkerThread.Terminate();
  SetEvent(WorkerThread.FWorkEvent);
  LRef := WorkerThread;
  WorkerThread := nil; //Will be freed usinf TThreaf.FreeOnTerminate
  if CanBlock then
    LRef.Free;
end;

Because of FreeOnTerminate = True, as soon as Terminate() has been called, WorkerThread can be disposed at any given time, so it is unsafe to access any of its fields or methods.

I realize that most of the times, this might not be a problem, because WorkerThread is stuck in WaitForSingleObject - but there's no guarantee for that.

To fix this, FWorkEvent should be set in an overridden TerminatedSet:

procedure TWorkerThread.TerminatedSet;
begin
  inherited;
  if not SetEvent(FWorkEvent) then
    RaiseLastOSError;
end;

With that, Dispose could be simplified to:

class procedure TWorkerThread.Dispose;
var
  LRef: TWorkerThread;
begin
  LRef:= WorkerThread;
  WorkerThread:= nil;
  LRef.FreeOnTerminate := not CanBlock;
  if LRef.FreeOnTerminate then
    LRef.Terminate() else
    LRef.Free;
end;

[joachimmarder]

I realize that most of the times, this might not be a problem, because WorkerThread is stuck in WaitForSingleObject - but there's no guarantee for that.

Why that? WorkerThread.Dispose() is called if the last treeview is destroyed. Before that, this treeview calls InterruptValidation(True), so the TWorkerThread cannot be validating at that moment.

Have you seen an actual problem with that code?

[modersohn]

Have you seen an actual problem with that code?

No, I most definitely did not, so you're of course free to not consider this a bug.

But your argumentation sort of demonstrates my point: the current code makes assumptions about when it is called and what state the thread is in at that moment. This can be avoided with the code I suggested, so it would be more robust about possible changes in the future that might invalidate any of those assumptions.

I actually came across this while trying to figure out what is causing the exceptions I see in the debugger with the new unit tests I wrote for #1001. Since these always relate to SetEvent and ResetEvent calls (in WorkerThread, but also in the VCL itself), I was doing a static code analysis for the event related code in WorkerThread and came across this. Not surprisingly, the changes had no effect and the exceptions are still there.

[joachimmarder]

the current code makes assumptions

I think this is quite normal and OK as long as this happens within a class like here.
I may be wrong, but I think your code would rely on the fact that the Terminated property is checked before (and after) the call of WaitForSingleObject(). Otherwise a race condition could occur that deadlocks the thread.

I was doing a static code analysis

Which tool did you use?

I actually came across this while trying to figure out what is causing the exceptions I see in the debugger with the new unit tests I wrote for #1001

Any chance you send a pull request so that I can take a look?

[modersohn]

I think this is quite normal and OK as long as this happens within a class like here.

Sure there's nothing inherently wrong here, but the less assumptions the code needs to make, the more resilient it is to possible future changes.

I may be wrong, but I think your code would rely on the fact that the Terminated property is checked before (and after) the call of WaitForSingleObject(). Otherwise a race condition could occur that deadlocks the thread.

Of course it assumes that the thread checks Terminated - that is pretty much a universal condition any thread has to adhere to. And BTW that same assumption also applies to the current code as well.

I also don't see how setting the event in that way could cause a race-condition - it pretty much does the same thing as the current code, but without the assumption that the thread is waiting by the time Terminate and then SetEvent are called.

An overridden TerminatedSet which sets an event is a very common code pattern that signals that the thread needs to be "woken up" from WaitForSingleObject to be able to terminate. In fact, TerminatedSet was explicitly added to TThread quite some time ago with that in mind (see also https://stackoverflow.com/a/33963443/386473).

Which tool did you use?

I didn't use a tool (and I'm not aware of one for Delphi code that is able to detect these sort of things) - in other words, it was just a fancy word for "looking at the code", but with a different mindset (what could possibly go wrong) and looking for familiar and unfamiliar code patterns.

Any chance you send a pull request so that I can take a look?

You already have the test code in https://github.com/JAM-Software/Virtual-TreeView/blob/master/Tests/VTWorkerThreadIssue1001Tests.pas. If I run this in the IDE with Win32, I see exceptions on the SetEvent call in AddTree, but also in TCustomForm.BeforeDestruction in GlobalNameSpace.BeginWrite which eventually calls ResetEvent. If you don't see any of those, it might help to increase the number of repeats of the test and/or the number of nodes that are added. If you do see any of the exceptions, I'd suggest we open a new issue for this.

[joachimmarder]

If you don't see any of those, it might help to increase the number of repeats of the test and/or the number of nodes that are added.

I tried 500 iterations and 100k nodes, Win32 and Win64, but didn't see any exception. I used the latest code with some changes of yesterday evening.

[modersohn]

I tried 500 iterations and 100k nodes, Win32 and Win64, but didn't see any exception. I used the latest code with some changes of yesterday evening.

Okay thanks for letting me know - I was afraid that could happen, because the exceptions seem very odd in that they only happen when debugging and only on Win32. I also spent way too much time trying to figure out what's going on there, so it will have to remain a mystery for now, I'm afraid.

[Dzyszla]

Doesn't it correspond to #1245 ?

[joachimmarder]

Doesn't it correspond to #1245 ?

I think you are right. Since #1245 is closed, this may explain why I was unable to reproduce the failure. Anyway, I am closing this issue now, I'm quite sure that there is no problem left. We are using VTV heavily in our products at JAM Software, I think we would have noticed that.