[fpc-devel] Suspicion about TThread.Synchronize

[Sven Barth]

On 05.02.2018 03:03, Martin wrote:
> Ok, I got it again in the debugger.
> 
> Since it is nor reproducible all the times, I am using auto continue
> breakpoints, and just record that they were hit. So I have limited data.
> 
> - After a long series of syncs ThreadQueueTail is nil.
> - A thread calls ThreadQueueAppend (from within Syncronize)
> - It then leaves it do execute
> 
> there was no CheckSyncronize in between the 2 last steps. the thread
> should be waiting for the main thread to sync. but it does not.
> 
> I captured the SyncEvent of the queue entries. (as seen in
> checkSyncronize). And there something strange goes on.
> The queue entries from 2 threads have the same SyncEvent
> 
> Maybe this is actually an issue in the application code.
> 
> The below is a method on a TTHread. This method is called before the
> thread is started, it does actually start it (last line)
> 
> procedure TThreadDownload.DownloadJSON(const ATimeOut: Integer = -1;
>   const ASilent: Boolean = False);
> begin
>   FRemoteJSONFile :=
> Options.RemoteRepository[Options.ActiveRepositoryIndex] + cRemoteJSONFile;
>   FDownloadType := dtJSON;
>   FSilent := ASilent;
>   FTimer := TThreadTimer.Create;
>   FTimer.Interval := ATimeOut;
>   FTimer.OnTimer := @DoOnTimer;
>   FTimer.StartTimer;
>   Start;
> end;
> 
> So when the ThreadTimer goes off, it calls DoOnTimer. But on the
> TThreadDownload object
> 
> And in DoOnTimer makes a call to Syncronize. (but DoOnTimer is a method
> of TThreadDownload)
> 
> Am I right, and it then uses the ID of the TThreadDownload? Even so it
> is running in the thread of the ThreadTimer?
> 
> So if TThreadDownload schedules its own Synchronize, before the event of
> ThreadTimer is processed, the TThreadDownload will falsely think, that
> it's event was processed when in reality it was the event of the timer.
> 
> Also then probably the event in FSynchronizeEntry is used twice in
> parallel, which it is not designed for?

Good find! Thank you for digging down to this.

While this might indeed best be fixed in the application code (and
indeed it must be for correctly working on the 3.0.x series) I've
implemented a fix in r38124 that will harden Synchronize() a bit better
against such "abuse" (it will now execute the method using the correct
thread instance).

Regards,
Sven