[fpc-devel] Suspicion about TThread.Synchronize

[Sven Barth]

On 03.02.2018 17:39, Martin wrote:
> All based on win32
> 
> Pretext:
> I have an issue with a crash in PopThreadQueueHead called by
> CheckSynchronize.  (3.0.2)
> It happens in the Lazarus IDE, but at a low percentage only. (And not
> yet in the debugger)
> I don't think the below is related, but I found it while looking around.

Do you happen to have the exact location of the crash?

> Potential Issue: (3.0.2 and trunk 37936)
> 
> I have not looked very deep into the threading internals, so I may have
> missed something, but I thought I just bring it up for review.
> 
> rtl\objpas\classes\classes.inc
>      class procedure TThread.Synchronize(AThread: TThread; AMethod:
> TThreadMethod);
> contains those lines (outside a CriticalSection):
>     ThreadQueueAppend(AThread.FSynchronizeEntry);
> 
>     AThread.FSynchronizeEntry^.Method := Nil;
>     AThread.FSynchronizeEntry^.Next := Nil;   // outside of critical
> section
> 
> 
> ThreadQueueAppend will enter a CriticalSection.
> It will add the passed in FSynchronizeEntry (param aEntry) to
> ThreadQueueHead/Tail. For this it may use another entries Next pointer.
> 
> So what happens, if 2 (or more) threads call TThread.Synchronize?
> 
> What happens if
> - thread 1, has just finished the line
> ThreadQueueAppend(AThread.FSynchronizeEntry);
> - thread 2 now calls synchronize, and
>    - as soon as thread1 leaves the CriticalSection in ThreadQueueAppend
> => thread2 enters that CriticalSection
> - thread2 adds it entry to the list, assigning it the Next^ of thread
> 1's entry
> - now thread1 continues, it sets Next^ to nil, removing the reference to
> thread 2's entry.
> 
> Thread 2 would no longer be in the list.
> That also means, if thread 2's entry has a Next^, then this may not be
> set to nil, if the entry it points to is removed?

The queue head is only modified inside ThreadQueueAppend() and
PopThreadQueueHead() (well, and RemoveQueuedEvents()) of which all use
the ThreadQueueLock while working on the queue. Once ThreadQueueAppend()
inside Synchronize() is executed the event is already processed (because
ThreadQueueAppend() is a blocking call in that case) and the work on
AThread.FSynchronizeEntry is merely cleanup to avoid any misbehaviour
(at that time the entry is already no longer part of the queue).

Regards,
Sven