Watch, Follow, &
Connect with Us
Public Report
Report From: Delphi-BCB/RTL/Delphi/Thread support    [ Add a report in this area ]  
Report #:  83212   Status: Closed
Incorrect TThread.Synchronize behavior
Project:  Delphi Build #:  8.1
Version:    7.0 Submitted By:   Oleg Gusakov
Report Type:  Crash / Data loss / Total failure Date Reported:  3/22/2010 2:08:56 AM
Severity:    Extreme corner case Last Updated: 3/22/2010 7:04:52 AM
Platform:    All platforms Internal Tracking #:  
Resolution: Test Case Error (Resolution Comments) Resolved in Build: : None
Duplicate of:  None
Voting and Rating
Overall Rating: (1 Total Rating)
5.00 out of 5
Total Votes: None
Description
The TThread.Synchronize method uses internal FSynchronize field to store information about a calling method. But Synchronize can also been called from other thread in the same time. In this case FSynchronize field will be overwritten and one of the two calls will be called twice.
Steps to Reproduce:
None
Workarounds
Remove FSynchronize and make it local in method:

procedure TThread.Synchronize(Method: TThreadMethod);
var
  Sync: TSynchronizeRecord;
begin
  Sync.FThread := Self;
  Sync.FSynchronizeException := nil;
  Sync.FMethod := Method;
  Synchronize(@Sync);
end;
Attachment
TestSync2.zip
Comments

Tomohiro Takahashi at 3/22/2010 2:51:28 AM -
> ... FSynchronize field will be overwritten and one of the two calls will be called twice.
Could you please attach sample project to reproduce your issue?

Oleg Gusakov at 3/22/2010 3:00:59 AM -
Hmm... I attach but seems it lost when submit... ok, I try to fix

Tomohiro Takahashi at 3/22/2010 7:07:18 AM -
Does documentation say that Synchronize method of TThread class is thread-safe? Generally speaking, as you know, we can call a method which is said 'thread-safe' in document from multiple threads without lock.

In addtition, please see documentation about TThread.Synchronize.
http://docwiki.embarcadero.com/VCL/en/Classes.TThread.Synchronize

Oleg Gusakov at 3/22/2010 8:01:52 AM -
Yes, documentation says that Synchronize should not be called from main thread. Actually I don't know why since it include necessary protection:

class procedure TThread.Synchronize(ASyncRec: PSynchronizeRecord);
var
  SyncProc: TSyncProc;
begin
  if GetCurrentThreadID = MainThreadID then
    ASyncRec.FMethod
  else
    . . .
end;

Anyway, calling Synchronize from main thread can occurs in some "abstract layer" code that potentially can run under different threads and make calls Synchronize(OnSomeEvent) just for safe. If in the main thread we can't do it then we need to add thread checking in this "abstract" code, exactly the same like existed in the Synchronize.

Tomohiro Takahashi at 3/24/2010 6:14:20 PM -
> ... documentation says that Synchronize should not be called ...
No, it says 'Do not call Synchronize from within the main thread.'.

Dick Boogaers at 3/25/2010 3:54:36 AM -
In English there is hardly any difference between these two statements.

Tomohiro Takahashi at 3/22/2010 7:48:37 PM -
Could you please try to lock/unlock a TThread instance properly to avoid confliction in main thread and worker thread?

Oleg Gusakov at 3/23/2010 3:13:36 AM -
Sorry, but what conflict you mean? The test project doesn't contain any locks (except internal in Synchronize). Can you please give me more details about problem with it?

Tomohiro Takahashi at 3/24/2010 12:14:02 AM -
> but what conflict you mean?
Sorry, it is just a idea...
First of all, under main thread context, you do not need to call TThread.Synchronize method.
And, access modifier of TThread.Synchronize method is 'protected'. So, generally speaking, you are not allowed to call the protected method from TForm class which is not sub class of TThread.

If you need more support, please contact technical support service.
http://support.embarcadero.com/

Dick Boogaers at 3/24/2010 3:31:45 AM -
Oleg just made a quick example to show the problem. And it is a terrific bug!

Why don't you reopen this report, cause this bug should not continue to exist.

I think you're a bit too quick in closing this report: wait until the originator had a chance to clarify things.

Tomohiro Takahashi at 3/24/2010 6:13:30 PM -
Thanks for the notification.
But, according to documentaion of TThread.Synchronize, it says Do not call Synchronize from within the main thread.

Dick Boogaers at 3/25/2010 3:57:26 AM -
Oleg argues that it will also go wrong if the Synchronize call is not done from the main thread. That's why I asked you to wait with closing the report: the issue is not resolved yet.

By the way, I greatly appreciate your work for QC.

Oleg Gusakov at 3/24/2010 1:45:19 AM -
Ok, here is absolutely correct example that makes the same:

type
  TSomeThread = class(TThread)
  protected
    procedure InternalSomeMethod;
  public
    procedure SomeMethod;
  end;

procedure TSomeThread.InternalSomeMethod;
begin
  // Do things in the main thread only
end;

procedure TSomeThread.SomeMethod;
begin
  Synchronize(InternalSomeMethod);
end;

The SomeMethod can be called both from the main thread and from TSomeThread.Execute. Such code have the same potential problem with FSynchronize overwrite.

Tomohiro Takahashi at 3/22/2010 4:35:50 AM -
In your code, is your issue caused by calling TThread.Synchronize under main thread context as below?
> procedure TForm1.WMUser(var Message: TMessage);
> begin
>   FThread.Synchronize(MainSyncMethod);
> end;

Oleg Gusakov at 3/22/2010 7:22:44 AM -
Yes, it calls from main thread.

Actually in first time I'm not take into account that Synchronize(Method) it is *protected* method and normally should call by the thread methods only. Public overloaded declarations needs two parameters AThread and AMethod (it also uses correct call of the private Synchronize method). But in my case thread declared in Main.pas and main form can also calls protected Synchronize() method with one parameter.

But I still think that FSynchronize field is useless and potentially may cause an error (as in this example).

Dick Boogaers at 3/22/2010 4:32:14 AM -
Perfect find! Excellent report. Thanks.

Remy Lebeau (TeamB) at 7/20/2010 2:15:44 AM -
Please re-open this bug.  It just surfaced inside of Indy 10 and caused some big headaches.

In short, Indy's TIdSync and TIdNotify classes call the non-static TThread.Synchronize() method on a global TThread object.  So it is possible for multiple worker threads to call TThread.Synchronize() on a single TThread object at the same time, thus overwriting the FSynchronize field data.

Indy is being patched to call the static version of TThread.Synchronize() from now on (and adding new calls to TThread.Queue()).  But even then, the static TThread.Synchronize() method is publically accessible in general and its AThread parameter allows a common TThread object to be shared by multiple callers, so this is still a real-world bug that really needs to be fixed in TThread by eliminating the FSynchronize field altogether, as is not doing anything useful.

Tomohiro Takahashi at 7/20/2010 8:03:52 PM -
> Please re-open this bug.  It just surfaced inside of Indy 10 and caused some big headaches.
Remy-san
Could you please put new report about TThread.Synchronize and Indy 10? I will check it out ASAP.

Dick Boogaers at 7/21/2010 2:50:41 AM -
This is not something that can easily be tested. It depends on your computer configuration, since it is extremely timing dependend.

I think the solution is simple, and is already demonstrated in the overloaded class procedure TThread.Synchronize(AThread: TThread; AMethod: TThreadMethod).

procedure TThread.Synchronize(Method: TThreadMethod);
var
  SyncRec: TSynchronizeRecord;
begin
  SyncRec.FThread := Self;
  SyncRec.FSynchronizeException := nil;
  SyncRec.FMethod := Method;
  Synchronize(@SyncRec);
end;

Server Response from: ETNACODE01