[SOLVED] Thread Safety?

[JD]

@ASerge
I tried your code on linux (under debugger), and if the timer interval is set low enough (~30 or below) the progressbar does reach its Max.
However, the form remains unresponsive, and the app has to be killed.
Have I missed a vital piece?

I can confirm this also. It stops at 98.2%.

I already solved the hanging at the end.
See my post earlier.

(FreeAndNil(FThread); should never be called in its own event-handler. And FThread.OnTerminate is an event-handler.)

Gotcha rvk. I didn't see that.

[rvk]

no processmessages is needed.

Thank you very much for explanations. It is very informatively. I think that not only I, but also many other people would be grateful to you if you would lay out the minimum demo-project here. 

OK here you go a minimalistick sample of multihreading with out processmessages simple and to the point. I have added a couple of try/excepts/finally that showcase a more stable execution path and moved the thread code to a secondary method for simplicity's shake alone.

@taazz, What's the upside of using Application.QueueAsyncCall() over the threads own TThread.Queue()?

O, wait, maybe it's that the TThread.Queue() will be deleted when the TThread exits prematurely and in that case the FreeMem for the reserved data isn't called.

Otherwise, if you don't need to reserve memory, calling TThread.Queue() will be fine too, I think.

[taazz]

@taazz, What's the upside of using Application.QueueAsyncCall() over the threads own TThread.Queue()?

Familiarity. I start using it from lazarus v 1.0 it did not had thread.queue at that time. I keep using it because it helps me visualize which thread will execute the queued method a fact that is lost on thread.queue.

O, wait, maybe it's that the TThread.Queue() will be deleted when the TThread exits prematurely and in that case the FreeMem for the reserved data isn't called.

no. You are confusing the owner of the Queue with a class method which can be used with and with out an instantiated object. The methods will be executed and their memory freed regardless of the state of the thread.

Otherwise, if you don't need to reserve memory, calling TThread.Queue() will be fine too, I think.

The main pro of a thread.queue is that it does not need a GUI application.

[rvk]

O, wait, maybe it's that the TThread.Queue() will be deleted when the TThread exits prematurely and in that case the FreeMem for the reserved data isn't called.

no. You are confusing the owner of the Queue with a class method which can be used with and with out an instantiated object. The methods will be executed and their memory freed regardless of the state of the thread.
...
When a thread finishes it's execution, all its queued calls are removed from the queue list.

Yes. All call are removed from the queue list. But does that mean they are EXECUTED ??
If I put a FForm.Update in the queue after doing New(vData); does that mean the FForm.Update is ALWAYS executed?

I read that the calls are removed from the queue list. NOT that they are executed.

When a thread finishes it's execution, all its queued calls are removed from the queue list.

https://www.freepascal.org/docs-html/rtl/classes/tthread.queue.html

And if they are not executed, you'll get a memory leak because the corresponding FreeMem() isn't called in FForm.Update.

Edit: If I look in the source, I can only find that the queueentries are removed. Not that the corresponding call is executed. So with TThread.Queue() you can't be absolutely sure the call is being made. Unless you do a dummy Synchronize(Dummyproc) at the end of your Execute procedure. In that case all queues are empties by executing the calls. It will delay the end of the thread slightly. (something like this: https://stackoverflow.com/a/40034987/1037511)

[taazz]

O, wait, maybe it's that the TThread.Queue() will be deleted when the TThread exits prematurely and in that case the FreeMem for the reserved data isn't called.

no. You are confusing the owner of the Queue with a class method which can be used with and with out an instantiated object. The methods will be executed and their memory freed regardless of the state of the thread.
...
When a thread finishes it's execution, all its queued calls are removed from the queue list.

Yes. All call are removed from the queue list. But does that mean they are EXECUTED ??
If I put a FForm.Update in the queue after doing New(vData); does that mean the FForm.Update is ALWAYS executed?

I read that the calls are removed from the queue list. NOT that they are executed.

now that you mention it you are right I remember the same discussion in this forum I ranted a bit about that. I haven't done much on the subject since then though, mostly because all my pascal projects are in service mode (aka laz 1.4.4) or be converted to C#.

When a thread finishes it's execution, all its queued calls are removed from the queue list.

https://www.freepascal.org/docs-html/rtl/classes/tthread.queue.html

And if they are not executed, you'll get a memory leak because the corresponding FreeMem() isn't called in FForm.Update.

Edit: If I look in the source, I can only find that the queueentries are removed. Not that the corresponding call is executed. So with TThread.Queue() you can't be absolutely sure the call is being made. Unless you do a dummy Synchronize(Dummyproc) at the end of your Execute procedure. In that case all queues are empties by executing the calls. It will delay the end of the thread slightly. (something like this: https://stackoverflow.com/a/40034987/1037511)

If I recall correctly you simply set the thread parameter to the main thread to avoid this problem.

[rvk]

If I recall correctly you simply set the thread parameter to the main thread to avoid this problem.

Ah, yes, that's it. I can also recall the discussion about that (It's here and further).

Calling Queue with the main-thread does have the downside that you don't have the threads parameter in the call (so you can't access it's properties in the synced-call). But that's a miner issue compared to a possible crash (or memory leak).

Application.QueueAsyncCall() is more clear in that case. You know it belongs to the main-thread.

Aaah... thread-programming... So much to consider... But if done right, a powerful tool

[howardpc]

After considering engkin's comments, the following adaptation of ASerge's example (removing what is not needed) seems to work well.

Code: Pascal  [Select][+][-]

1. unit Unit1;
2.  
3. {$mode objfpc}{$H+}
4.  
5. interface
6.  
7. uses
8.   Classes, SysUtils, Forms, StdCtrls, ComCtrls, ExtCtrls;
9.  
10. type
11.  
12.   TProgressThread = class(TThread)
13.   private
14.     FInfo: string;
15.     FMax: Integer;
16.     FProgress: Integer;
17.   protected
18.     procedure Execute; override;
19.   public
20.     constructor Create(AMax: Integer);
21.     property Progress: Integer read FProgress;
22.     property Info: string read FInfo;
23.   end;
24.  
25.   TForm1 = class(TForm)
26.     Label1: TLabel;
27.     ProgressBar1: TProgressBar;
28.     Timer1: TTimer;
29.     procedure FormCreate(Sender: TObject);
30.     procedure FormDestroy(Sender: TObject);
31.     procedure Timer1Timer(Sender: TObject);
32.   private
33.     FThread: TProgressThread;
34.     procedure ThreadTerminated(Sender: TObject);
35.   public
36.     procedure SynchronizeGUI;
37.   end;
38.  
39.   var
40.     Form1: TForm1;
41.  
42.   implementation
43.  
44.   {$R *.lfm}
45.  
46.   constructor TProgressThread.Create(AMax: Integer);
47.   begin
48.     inherited Create(False);
49.     FMax := AMax;
50.   end;
51.  
52.   procedure TProgressThread.Execute;
53.   begin
54.     while not Terminated and (FProgress <= FMax) do
55.     begin
56.       Sleep(10); // Some long work
57.       Inc(FProgress);
58.       FInfo := Format('Pos: %d, Total: %.1f%%', [FProgress, FProgress * 100/FMax]);
59.     end;
60.   end;
61.  
62.   procedure TForm1.ThreadTerminated(Sender: TObject);
63.   begin
64.     ProgressBar1.Position := ProgressBar1.Max;
65.     Label1.Caption := 'Complete!';
66.   end;
67.  
68.   procedure TForm1.Timer1Timer(Sender: TObject);
69.   begin
70.     if Assigned(FThread) then
71.       FThread.Synchronize(@SynchronizeGUI);
72.   end;
73.  
74.   procedure TForm1.FormCreate(Sender: TObject);
75.   begin
76.     ProgressBar1.Max := 2000;
77.     FThread := TProgressThread.Create(ProgressBar1.Max);
78.     FThread.OnTerminate := @ThreadTerminated;
79.   end;
80.  
81.   procedure TForm1.FormDestroy(Sender: TObject);
82.   begin
83.     if Assigned(FThread) then
84.     begin
85.       FThread.OnTerminate := Nil; // Exclude GUI
86.       FThread.Free;
87.     end;
88.   end;
89.  
90.   procedure TForm1.SynchronizeGUI;
91.   begin
92.     ProgressBar1.Position := FThread.Progress;
93.     Label1.Caption := FThread.Info;
94.   end;
95.  
96.   end.

I also find that updating ProgressBar1.Position and Label1.Caption directly in the OnTimer call (i.e. not calling FThread.Synchronize at all) also works.
Is this a fluke, or a crash waiting to happen? Or can FThread and the GUI main thread get along in this fashion as a rule?

[engkin]

Still not correct.

[taazz]

Is this a fluke, or a crash waiting to happen?

not that's not a fluke, synchronize is a method to push data from a thread to the main thread while the ontimer event already executes in the mainthread it does not need to be called inside synchronize. To put it simple syncronize = push data to main thread, timer = pull data from a secondary thread. the second method does not require a call to synchronize but it does require protecting the data from corruption from both threads aka TProgressThread and main thread.

[rvk]

Is this a fluke, or a crash waiting to happen?

not that's not a fluke, synchronize is a method to push data from a thread to the main thread while the ontimer event already executes in the mainthread it does not need to be called inside synchronize. To put it simple syncronize = push data to main thread, timer = pull data from a secondary thread. the second method does not require a call to synchronize but it does require protecting the data from corruption from both threads aka TProgressThread and main thread.

Yes, if the main thread is "pushing" the data to the main thread, it doesn't change the string-variable at that point.

But when the TTimer decides itself when to pull the string, it could be that the thread is updating that string at the exact same time.

So...? The TSimpleRWSync/CriticalSection engkin was talking about.
But that's only needed when not using FThread.Synchronize(), I think.

It does feels a bit odd to call FThread.Synchronize() from the main thread in a TTimer.

[taazz]

Is this a fluke, or a crash waiting to happen?

not that's not a fluke, synchronize is a method to push data from a thread to the main thread while the ontimer event already executes in the mainthread it does not need to be called inside synchronize. To put it simple syncronize = push data to main thread, timer = pull data from a secondary thread. the second method does not require a call to synchronize but it does require protecting the data from corruption from both threads aka TProgressThread and main thread.

Yes, if the main thread is "pushing" the data to the main thread, it doesn't change the string-variable at that point.

true, synchronize makes sure that the method does not return until after the main thread has executed/accessed the data so no way to corrupt them like that.

But when the TTimer decides itself when to pull the string, it could be that the thread is updating that string at the exact same time.

So...? The TSimpleRWSync/CriticalSection engkin was talking about.
But that's only needed when not using FThread.Synchronize(), I think.

It does feels a bit odd to call FThread.Synchronize() from the main thread in a TTimer.

erm synchronize called from the main thread should execute immediately to avoid deadlocking it self, since the thread that is paused is the thread that called the synchronize method not the secondary thread that the variable points to. in other words if you use a timer the rwlock that aserge used in the original post is mandatory the synchronize call is just an cpu waster doesn't add anything to the process.

[rvk]

Yes, that is what I meant. rwlock is not needed if you use FThread.Synchronize() and visa versa.

But I do find it a strange construct to call FThread.Synchronize() in the TTimer of the main thread. It does works but I would normally prefer to call the Synchronize() in the thread itself with a time-check if needed (or every x number of iterations).

In the last given code the TTimer is even executed when the thread is already finished. But because the FreeAndNil is deleted, the Assigned(FThread) is true so the FThread.Synchronize is still called. Doesn't seem right. To fix that the TTimer needs to be stopped in ThreadTerminated(). That's why I don't like a TTimer in this fashion.

[taazz]

Yes, that is what I meant. rwlock is not needed if you use FThread.Synchronize() and visa versa.

Ok I'll assume you mean to call synchronize from the secondary thread only.

But I do find it a strange construct to call FThread.Synchronize() in the TTimer of the main thread. It does works but I would normally prefer to call the Synchronize() in the thread itself with a time-check if needed (or every x number of iterations).

its inappropriate use agreed. for all intense and purposes that should be considered a deadlock.

In the last given code the TTimer is even executed when the thread is already finished. But because the FreeAndNil is deleted, the Assigned(FThread) is true so the FThread.Synchronize is still called. Doesn't seem right. To fix that the TTimer needs to be stopped in ThreadTerminated(). That's why I don't like a TTimer in this fashion.

A few cpu wasters is the least of the problems but I see your point.

[sam707]

you can use (preferably for gauges and any progress display) TThread.Queue(...) instead of TThread.Synchronize(...)

the difference is that 'Queue' truely pushes methodS to be executed in main thread and returns immediatly so the running caller thread doesn't suffer of pauses while 'Synchronize' do a pause inside the caller thread

Let say 'Synchronize' is not a 'pusher' BUT a 'bottleneck pauser' "if multiples methods calls" while 'Queue' can be a 'non blocking true pusher'

you'll gain noticeable speed if your UI needs to display 3-xx gauges from 3-xx threads hehehehe

[ASerge]

Made some changes to my last example:
1. As noted by @rvk, threads should not be terminated inside their events. So changed FreeAndNil with FThread := nil, and added FreeOnTerminate := True.
2. The procedure Sleep allows system to switch the threads. A more realistic situation is when the thread itself performs continuous work. Replaced with SpinWait. And Max increased, otherwise it's too fast.
3. If the main thread uses shared variables, then inside the TestThread we can skip changes them right now. It is possible next time, all the same already became not actual. Therefore, not unconditional lock but just an attempt. Because IReadWriteSync does not contain the required function, it is replaced by the use of TRTLCriticalSection (TryEnterCriticalSection).
4. To reduce the time for locking the shared variables inside OnTimer, copy them to local ones, and then apply them to the controls.
5. As noted by @engkin, "Calling Terminate and WaitFor is redundant as TThread.Free leads to calling both", so replace it with FThread.Free (this works for nil as well).
6. Set Timer interval to 200ms. It's pretty fast from the human point of view, but very rarely, from the processor's point of view. In addition eliminated situation, when common variables are requested in OnTimer more often than they change in the TestThread, otherwise the TryEnterCriticalSection call can never be executed.
7. And in the end, microoptimization, due to the replacement of Format with FmtStr, eliminates the try finally block for the temporary string variable.

Code: Pascal  [Select][+][-]

1. unit Unit1;
2.  
3. {$mode objfpc}{$H+}
4.  
5. interface
6.  
7. uses
8.   Classes, SysUtils, Forms, Controls, ComCtrls, ExtCtrls, StdCtrls;
9.  
10. type
11.   TTestThread = class(TThread)
12.   private
13.     FInfo: string;
14.     FMax: Integer;
15.     FProgress: Integer;
16.     FCriticalSection: TRTLCriticalSection;
17.   protected
18.     procedure Execute; override;
19.   public
20.     constructor Create(AMax: Integer; ACriticalSection: TRTLCriticalSection);
21.     property Progress: Integer read FProgress;
22.     property Info: string read FInfo;
23.   end;
24.  
25.   TForm1 = class(TForm)
26.     Label1: TLabel;
27.     ProgressBar1: TProgressBar;
28.     Timer1: TTimer;
29.     procedure FormCreate(Sender: TObject);
30.     procedure FormDestroy(Sender: TObject);
31.     procedure Timer1Timer(Sender: TObject);
32.   private
33.     FCriticalSection: TRTLCriticalSection;
34.     FThread: TTestThread;
35.     procedure ThreadTerminated(Sender: TObject);
36.   end;
37.  
38. var
39.   Form1: TForm1;
40.  
41. implementation
42.  
43. {$R *.lfm}
44.  
45. constructor TTestThread.Create(AMax: Integer; ACriticalSection: TRTLCriticalSection);
46. begin
47.   inherited Create(False);
48.   FMax := AMax;
49.   FCriticalSection := ACriticalSection;
50. end;
51.  
52. procedure TTestThread.Execute;
53. begin
54.   while not Terminated and (FProgress <= FMax) do
55.   begin
56.     SpinWait(7000); // Hide some real work
57.     if TryEnterCriticalsection(FCriticalSection) <> 0 then
58.     try
59.       Inc(FProgress);
60.       FmtStr(FInfo, 'Pos: %d, Total: %.1f%%', [FProgress, FProgress * 100/ FMax]);
61.     finally
62.       LeaveCriticalsection(FCriticalSection);
63.     end;
64.   end;
65. end;
66.  
67. procedure TForm1.ThreadTerminated(Sender: TObject);
68. begin
69.   FThread := nil;
70.   ProgressBar1.Position := ProgressBar1.Max;
71.   Label1.Caption := 'Complete!';
72. end;
73.  
74. procedure TForm1.Timer1Timer(Sender: TObject);
75. var
76.   LPosition: Integer;
77.   LCaption: string;
78. begin
79.   if Assigned(FThread) then
80.   begin
81.     EnterCriticalsection(FCriticalSection);
82.     try
83.       LPosition := FThread.Progress;
84.       LCaption := FThread.Info;
85.     finally
86.       LeaveCriticalsection(FCriticalSection);
87.     end;
88.     ProgressBar1.Position := LPosition;
89.     Label1.Caption := LCaption;
90.   end
91.   else
92.     Timer1.Enabled := False;
93. end;
94.  
95. procedure TForm1.FormCreate(Sender: TObject);
96. begin
97.   ProgressBar1.Max := 200000;
98.   Timer1.Interval := 200;
99.   InitCriticalSection(FCriticalSection);
100.   FThread := TTestThread.Create(ProgressBar1.Max, FCriticalSection);
101.   FThread.FreeOnTerminate := True;
102.   FThread.OnTerminate := @ThreadTerminated;
103. end;
104.  
105. procedure TForm1.FormDestroy(Sender: TObject);
106. begin
107.   FThread.Free;
108.   DoneCriticalsection(FCriticalSection);
109. end;
110.  
111. end.

Please check in Linux.

In my opinion, this is a handy scenario when the work thread performs a lot of work, with almost no delays, while recording the current state in shared variables as quickly as it can. And the main thread displays the state of this data, but in order not to load the processor, it does it slowly (with the help of a timer), but visually fast enough.