[SOLVED] Thread Safety?

[ASerge]

Or use synchronizing objects. Example:

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.     FSync: IReadWriteSync;
17.   protected
18.     procedure Execute; override;
19.   public
20.     constructor Create(AMax: Integer; const ASync: IReadWriteSync);
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.     FSync: IReadWriteSync;
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; const ASync: IReadWriteSync);
46. begin
47.   inherited Create(False);
48.   FMax := AMax;
49.   FSync := ASync;
50. end;
51.  
52. procedure TTestThread.Execute;
53. begin
54.   while not Terminated and (FProgress <= FMax) do
55.   begin
56.     Sleep(10); // Some long work
57.     FSync.BeginWrite;
58.     try
59.       Inc(FProgress);
60.       FInfo := Format('Pos: %d, Total: %.1f%%', [FProgress, FProgress * 100/ FMax]);
61.     finally
62.       FSync.EndWrite;
63.     end;
64.   end;
65. end;
66.  
67. procedure TForm1.ThreadTerminated(Sender: TObject);
68. begin
69.   FreeAndNil(FThread);
70.   ProgressBar1.Position := ProgressBar1.Max;
71.   Label1.Caption := 'Complete!';
72. end;
73.  
74. procedure TForm1.Timer1Timer(Sender: TObject);
75. begin
76.   if Assigned(FThread) then
77.   begin
78.     FSync.BeginRead;
79.     try
80.       ProgressBar1.Position := FThread.Progress;
81.       Label1.Caption := FThread.Info;
82.     finally
83.       FSync.EndRead;
84.     end;
85.   end;
86. end;
87.  
88. procedure TForm1.FormCreate(Sender: TObject);
89. begin
90.   ProgressBar1.Max := 2000;
91.   FSync := TSimpleRWSync.Create as IReadWriteSync;
92.   FThread := TTestThread.Create(ProgressBar1.Max, FSync);
93.   FThread.OnTerminate := @ThreadTerminated;
94. end;
95.  
96. procedure TForm1.FormDestroy(Sender: TObject);
97. begin
98.   if Assigned(FThread) then
99.   begin
100.     FThread.OnTerminate := nil; // Exclude GUI
101.     FThread.Terminate; // Signal to stop
102.     FThread.WaitFor;
103.     FThread.Free;
104.   end;
105. end;
106.  
107. end.

[zoltanleo]

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. 

[ASerge]

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. 

I took an empty project, added ProgressBar and Timer. Formed events OnCreate, OnDestroy and Timer.OnTimer. Source code is provided earlier.

[knuckles]

Yes I have also heard a lot of bad things about Application.ProcessMessages,

I will take more time to read these follow up messages and examples. I was going to attempt something today but glancing at what has been posted I think I would have ended off track, so thanks again everyone for your input I will study what has been posted and play around some more

The main thing I was interested in was ensuring I made my threads safe and don't violate coding standards etc, of course seeing examples helps a lot to know what the syntax and things look like too.

[howardpc]

@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?

[rvk]

Code of ASerge works fine for me on Windows.
Attached a compilable test-project.

[ASerge]

@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.

I don't know. Have windows only.
Try to replace

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

1. Label1.Caption := FThread.Info;

with

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

1. S := FThread.Info; Label1.Caption := S;

Maybe the library takes a long time to transfer the string to the control, and the thread has already been freed?
Or replace

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

1. Label1.Caption :=

with

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

1. Caption :=

[howardpc]

Unfortunately neither of your suggestions made any difference, either to my implementation of your code, or to rvk's attached project (which is virtually identical to mine), when run under linux.

I can only think there is some incompatibility with IReadWriteSync and "uses cthreads".

[rvk]

Howard, yes. I have installed Mint 18.3 in a VM and I see the same.
Although the form is responsive, the progress bar advances with great leaps and the program hangs at the end.

Why is UseCThreads not default defined for Linux? I commented out the IFDEF now but I thought this should be default.

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

1. uses
2.   {$IFDEF UNIX}{.$IFDEF UseCThreads}
3.   cthreads,
4.   {.$ENDIF}{$ENDIF}
5.  

For the great leaps the ProgressBar1.Smooth should be set to true.
After that the TTimer.Inverval can be set to a lower frequency, like 100.
The hanging at the end is due to the FreeAndNil(FThread).
I thought you should NEVER EVER free something in it's own even-handler (which is done here).
Comment it out and all works fine:

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

1. procedure TForm1.ThreadTerminated(Sender: TObject);
2. begin
3.   // FreeAndNil(FThread);
4.   ProgressBar1.Position := ProgressBar1.Max;
5.   Label1.Caption := 'Complete!';
6. end;

[taazz]

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.

[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%.

[engkin]

for simplicity's shake alone.

That must be elegant.

[taazz]

for simplicity's shake alone.

That must be elegant.

or painful

[rvk]

@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.)

[engkin]

@ASerge
Thanks for the sample.

• Threads should not be terminated inside their events (Thanks rvk)
• Using TSimpleRWSync/CriticalSection means threads might pause waiting for other threads. Not needed here.
• Calling Terminate and WaitFor is redundant as TThread.Free leads to calling both, AFAIR.