Recent

Author Topic: [SOLVED] Thread Safety?  (Read 22335 times)

knuckles

  • Full Member
  • ***
  • Posts: 122
[SOLVED] Thread Safety?
« on: February 20, 2018, 08:46:35 pm »
I want to learn how to use TThreads so the UI doesn't lock up when running cpu intensive tasks, below I made my first attempt at creating a thread:

Code: Pascal  [Select][+][-]
  1. unit Unit1;
  2.  
  3. {$mode objfpc}{$H+}
  4.  
  5. interface
  6.  
  7. uses
  8.   Classes, SysUtils, FileUtil, Forms, Controls, Graphics, Dialogs, StdCtrls,
  9.   ComCtrls;
  10.  
  11. type
  12.   TForm1 = class(TForm)
  13.     Button1: TButton;
  14.     Label1: TLabel;
  15.     ProgressBar1: TProgressBar;
  16.     procedure Button1Click(Sender: TObject);
  17.   private
  18.     //
  19.   public
  20.     //
  21.   end;
  22.  
  23.   TMyThread = class(TThread)
  24.   private
  25.     FProgress: Integer;
  26.   protected
  27.     procedure Execute; override;
  28.     procedure UpdateProgress;
  29.   end;
  30.  
  31. var
  32.   Form1: TForm1;
  33.  
  34. implementation
  35.  
  36. {$R *.lfm}
  37.  
  38. { TMyThread }
  39.  
  40. procedure TMyThread.Execute;
  41. var
  42.   I: Integer;
  43. begin
  44.   for I := 0 to 25000 do
  45.   begin
  46.     FProgress := I;
  47.     Synchronize(@UpdateProgress);
  48.   end;
  49. end;
  50.  
  51. procedure TMyThread.UpdateProgress;
  52. begin
  53.   Form1.ProgressBar1.Position := FProgress; // not thread safe?
  54.   Form1.Label1.Caption := FProgress.ToString; // not thread safe?
  55. end;
  56.  
  57. { TForm1 }
  58.  
  59. procedure TForm1.Button1Click(Sender: TObject);
  60. var
  61.   MyThread: TMyThread;
  62. begin
  63.   ProgressBar1.Max := 25000;
  64.   ProgressBar1.Position := 0;
  65.   Label1.Caption := '0';
  66.  
  67.   MyThread := TMyThread.Create(False);
  68. end;
  69.  
  70. end.

I'm not sure if I am doing this wrong though, I hear about thread safety with the VCL (or in this case LCL) and I look at my procedure UpdateProgress and I directly access controls on Form1 - Isn't this incorrect, and if so what changes should I make to ensure it is thread safe and won't crash?

Currently the program creates the thread and it runs without error when I click the button, it feels a little jittery but does seem to work, however I am unsure if my approach is correct or flawed?

Many thanks in advance :)
« Last Edit: February 24, 2018, 02:53:37 pm by knuckles »

taazz

  • Hero Member
  • *****
  • Posts: 5368
Re: Thread Safety?
« Reply #1 on: February 20, 2018, 08:58:39 pm »
yes and no, yes this is one way to safely update your form. no, you got some details wrong.
1) do not use any variables that you do not have direct control from any thread code, especially globals, aka stop using the Form1 variable, it might not update what you expect and have a crash.
2) Synchronise, although perfectly valid choice, is the worst choice you have if updates are rapid, it might be preferable to use Application.QueueAsyncCall just make sure that the call you queue is a method of the form you update not the thread and you are as safe as you can be.
Good judgement is the result of experience … Experience is the result of bad judgement.

OS : Windows 7 64 bit
Laz: Lazarus 1.4.4 FPC 2.6.4 i386-win32-win32/win64

rvk

  • Hero Member
  • *****
  • Posts: 6109
Re: Thread Safety?
« Reply #2 on: February 20, 2018, 09:11:08 pm »
Overall your code seems fine.

In addition to the remarks taazz already gave you need to realize that Synchronize() is called a lot of times. Besides using Application.QueueAsyncCall you could also use TThread.Queue in which case the call to your UpdateProgress is queued.

But calling UpdateProgress so many times (25000 in a short time) isn't really needed, is it? Why not call UpdateProgress only every 10th (or 100th) iteration? You won't see every number flashing on screen anyway (unless you have superhuman vision). So be careful when to call Synchronize, Application.QueueAsyncCall or TThread.Queue and don't call it a few hundred times a second.

knuckles

  • Full Member
  • ***
  • Posts: 122
Re: Thread Safety?
« Reply #3 on: February 20, 2018, 09:44:34 pm »
Thanks for the replies and information, my code was just a test, I will try and read more of what has been said and see if I can update it more appropriately in the next day or so and will come back hopefully with something that looks more correct and doesn't use Form1

zoltanleo

  • Sr. Member
  • ****
  • Posts: 486
Re: Thread Safety?
« Reply #4 on: February 20, 2018, 11:26:45 pm »
@knuckles

I will make bold to give advice that you have studied the mechanism of sending messages to the main thread by means of SendMessage/PostMessage. It will allow you to operate operation of application more flexibly. Then your code will look approximately so
Code: Pascal  [Select][+][-]
  1. unit Unit1;
  2.  
  3. {$mode objfpc}{$H+}
  4.  
  5. interface
  6.  
  7. uses
  8.   Classes, SysUtils, Forms, Controls, Graphics, Dialogs, ComCtrls, StdCtrls
  9.   , LCLIntf, LCLType, LMessages, LResources; //add this units
  10.  
  11. const
  12.   LM_MYMSG = LM_USER + $101;
  13.  
  14. type
  15.  
  16.   { TMyThread }
  17.  
  18.   TMyThread = class(TThread)
  19.   private
  20.     FThreadIsTerm: Boolean;
  21.   protected
  22.     procedure Execute; override;
  23.   public
  24.     constructor Create(CreateSuspended: Boolean);
  25.   end;
  26.  
  27.   { TForm1 }
  28.  
  29.   TForm1 = class(TForm)
  30.     Button1: TButton;
  31.     Label1: TLabel;
  32.     ProgressBar1: TProgressBar;
  33.     procedure Button1Click(Sender: TObject);
  34.     procedure LmMyMsg(var Msg: TLMessage); message LM_MYMSG;
  35.   private
  36.     FMyThread: TMyThread;
  37.     FProgress: Integer;
  38.     FCaption: String;
  39.   public
  40.  
  41.   end;
  42.  
  43. var
  44.   Form1: TForm1;
  45.  
  46. implementation
  47.  
  48. {$R *.lfm}
  49.  
  50. { TForm1 }
  51.  
  52. procedure TForm1.Button1Click(Sender: TObject);
  53. begin
  54.   if Assigned(FMyThread) then Exit;
  55.  
  56.   ProgressBar1.Max := 250;
  57.   ProgressBar1.Position := 0;
  58.   Label1.Caption := '0';
  59.   Button1.Enabled:= False;
  60.   try
  61.     FMyThread:= TMyThread.Create(True);
  62.  
  63.     while not FMyThread.FThreadIsTerm do
  64.     begin
  65.       ProgressBar1.Position:= FProgress;
  66.       Label1.Caption:= FCaption;
  67.       Application.ProcessMessages;
  68.       Sleep(50);
  69.     end;
  70.  
  71.   finally
  72.     FreeAndNil(FMyThread);
  73.     ProgressBar1.Position := 0;
  74.     Label1.Caption := '0';
  75.     Button1.Enabled:= True;
  76.   end;
  77. end;
  78.  
  79. procedure TForm1.LmMyMsg(var Msg: TLMessage);
  80. begin
  81.   FCaption:= String(Msg.lParam);
  82.   FProgress:= Msg.wParam;
  83. end;
  84.  
  85. { TMyThread }
  86.  
  87. procedure TMyThread.Execute;
  88. var
  89.   Msg: TLMessage;
  90.   i: Integer;
  91. begin
  92.   for i:= 0 to 250 do
  93.     begin
  94.       Msg.lParam:= NativeInt(IntToStr(i));//it's an artificial example, only for demonstration of transfer of a string
  95.       PostMessage(Form1.Handle,LM_MYMSG,WPARAM(i),LPARAM(Msg.lParam));
  96.       Sleep(50);
  97.     end;
  98.   FThreadIsTerm:= True;
  99. end;
  100.  
  101. constructor TMyThread.Create(CreateSuspended: Boolean);
  102. begin
  103.   inherited Create(CreateSuspended);
  104.   FreeOnTerminate:= False;
  105.   Priority:= tpLower;
  106.   FThreadIsTerm:= False;
  107.   Start;
  108. end;
  109. end.                  
  110.  
This code cross-platform. He will work also for Linux also  ;)
Win10 LTSC x64/Deb 11 amd64(gtk2/qt5)/Darwin Cocoa (Monterey):
Lazarus x32/x64 2.3(trunk); FPC 3.3.1 (trunk), FireBird 3.0.10; IBX by TonyW

Sorry for my bad English, I'm using translator ;)

rvk

  • Hero Member
  • *****
  • Posts: 6109
Re: Thread Safety?
« Reply #5 on: February 20, 2018, 11:49:30 pm »
Then your code will look approximately so
Some notes:
  • Why "while not FMyThread.FThreadIsTerm do" in the ButtonClick. That will keep the code running in Button1Click, eliminating the need for a TThread entirely. Do you really see a need for a thread in that Button1Click()?
  • Why use "while not FMyThread.FThreadIsTerm do" while there is FMyThread.WaitFor;
  • No need for FMyThread.FThreadIsTerm if you use FMyThread.WaitFor.
  • Don't use Application.Processmessages.
  • Somehow I seem to remember that you can't send a string with PostMessage() like you did. You'll need to do NewStr() and dispose of the string in the main thread. At least, that's what was needed in Delphi. Otherwise you'll end up with a pointer to a non existing string.

Synchronize, Queue and Application.QueueAsyncCall are all cross-platform. So there is no need to use PostMessage. And seeing the notes above, I wouldn't advice PostMessage for a beginner in threads.

howardpc

  • Hero Member
  • *****
  • Posts: 4144
Re: Thread Safety?
« Reply #6 on: February 21, 2018, 12:33:47 am »
  • Don't use Application.Processmessages.
  • Somehow I seem to remember that you can't send a string with PostMessage() like you did. You'll need to do NewStr() and dispose of the string in the main thread. At least, that's what was needed in Delphi. Otherwise you'll end up with a pointer to a non existing string.[/l]
Surely you have to use Application.ProcessMessages if you want the GUI to respond to the thread messages?


At least on linux FPC lets you pass a string using NativeInt(stringVar) as a LParam or WParam without apparent problems.



taazz

  • Hero Member
  • *****
  • Posts: 5368
Re: Thread Safety?
« Reply #7 on: February 21, 2018, 12:44:45 am »
  • Don't use Application.Processmessages.
  • Somehow I seem to remember that you can't send a string with PostMessage() like you did. You'll need to do NewStr() and dispose of the string in the main thread. At least, that's what was needed in Delphi. Otherwise you'll end up with a pointer to a non existing string.[/l]
Surely you have to use Application.ProcessMessages if you want the GUI to respond to the thread messages?
If you need to call it then you are doing it wrong. the idea of thread is to execute in parallel with the main application thread not in sync. if your code looks like
Code: Pascal  [Select][+][-]
  1. var
  2.   MyThread:TMythread;
  3. begin
  4.   mythread := TMythread.create(suspended);
  5.   try
  6.     MyThread.MainData := MyThreadData;
  7.     MyThread.Resume;
  8.     repeat
  9.       application.ProcessMessages;
  10.     until Mythread.Finished
  11.   finally
  12.     MyThread.Free;
  13.   end;
  14. end;
  15.  
then you missed the point. Rule number 1. never call processmessages. If you do you are doing it wrong. No ifs or buts.
At least on linux FPC lets you pass a string using NativeInt(stringVar) as a LParam or WParam without apparent problems.
a string is a managed type you never pass managed types between threads only memory locations managed data have the bad habit to get freed with out a notice.
Good judgement is the result of experience … Experience is the result of bad judgement.

OS : Windows 7 64 bit
Laz: Lazarus 1.4.4 FPC 2.6.4 i386-win32-win32/win64

rvk

  • Hero Member
  • *****
  • Posts: 6109
Re: Thread Safety?
« Reply #8 on: February 21, 2018, 12:47:22 am »
Surely you have to use Application.ProcessMessages if you want the GUI to respond to the thread messages?
The fact there is such a loop in buttonclick is already a design-fault. Of course you can "solve" that with Application.ProcessMessages but that's not the right way to do it.

Quote
At least on linux FPC lets you pass a string using NativeInt(stringVar) as a LParam or WParam without apparent problems.
That depends. If the string itself stays on that memory location (to which lparam is pointing) you will have no problems. But if the string is freed and/or the memory location is overwritten before the postmessage is handled, you have a.big problem. Trust me, I know. In fpc this shouldn't be any different. The string which is reserved in intostr will be out of scope after the postmessage and will be freed. You probably don't experience any problem with it because the postmessage is handled very fast but you can't guarantee that. It's just a heads up if you use this construction in your own program.

howardpc

  • Hero Member
  • *****
  • Posts: 4144
Re: Thread Safety?
« Reply #9 on: February 21, 2018, 01:01:28 am »
OK, points taken.
It would be helpful if you had the time to rewrite knuckles' little example, but without any design fault, and showing correctly how to pass a string via PostMessage in a cross-platform way.

taazz

  • Hero Member
  • *****
  • Posts: 5368
Re: Thread Safety?
« Reply #10 on: February 21, 2018, 01:09:28 am »
OK, points taken.
It would be helpful if you had the time to rewrite knuckles' little example, but without any design fault, and showing correctly how to pass a string via PostMessage in a cross-platform way.
I'll take a shot at it and probably post it tomorrow. Just one more small point, everything said in here are not multiprocess safe, especially if the processes have different memory managers, compilers etc, you will need extra steps to make sure that memory passed between them can be freed from any of them or preserved even if the process that allocated the memory exits.

PS. I'm assuming you mean zoltanleo's example since knuckle's example does not call processmessages.
« Last Edit: February 21, 2018, 01:18:57 am by taazz »
Good judgement is the result of experience … Experience is the result of bad judgement.

OS : Windows 7 64 bit
Laz: Lazarus 1.4.4 FPC 2.6.4 i386-win32-win32/win64

zoltanleo

  • Sr. Member
  • ****
  • Posts: 486
Re: Thread Safety?
« Reply #11 on: February 21, 2018, 06:19:10 am »
OK, points taken.
It would be helpful if you had the time to rewrite knuckles' little example, but without any design fault, and showing correctly how to pass a string via PostMessage in a cross-platform way.
+1

Don't use Application.Processmessages.
In the real application I use SplashForm certainly here
Win10 LTSC x64/Deb 11 amd64(gtk2/qt5)/Darwin Cocoa (Monterey):
Lazarus x32/x64 2.3(trunk); FPC 3.3.1 (trunk), FireBird 3.0.10; IBX by TonyW

Sorry for my bad English, I'm using translator ;)

taazz

  • Hero Member
  • *****
  • Posts: 5368
Re: Thread Safety?
« Reply #12 on: February 21, 2018, 06:28:28 am »
Don't use Application.Processmessages.
In the real application I use SplashForm certainly here
sorry I do not understand. As extra information, I never use Application.ProcessMessages not even in single thread mode. I never had the need for it.
Good judgement is the result of experience … Experience is the result of bad judgement.

OS : Windows 7 64 bit
Laz: Lazarus 1.4.4 FPC 2.6.4 i386-win32-win32/win64

zoltanleo

  • Sr. Member
  • ****
  • Posts: 486
Re: Thread Safety?
« Reply #13 on: February 21, 2018, 07:07:50 am »
sorry I do not understand. As extra information, I never use Application.ProcessMessages not even in single thread mode. I never had the need for it.
OK. Graphic elements of the main thread have to be redrawn at that time while other work becomes in an additional thread. Otherwise the LCL elements will be "frozen".

Therefore I have to show additional SplashForm with animation from above of the main window. Or I have to redraw somehow LCL elements of the main window while the additional thread performs work. For this purpose I use Application.ProcessMessages.

If it isn't correct, then I will ask to set the "correct" example without application of ProcessMessages  :-[
« Last Edit: February 21, 2018, 07:14:52 am by zoltanleo »
Win10 LTSC x64/Deb 11 amd64(gtk2/qt5)/Darwin Cocoa (Monterey):
Lazarus x32/x64 2.3(trunk); FPC 3.3.1 (trunk), FireBird 3.0.10; IBX by TonyW

Sorry for my bad English, I'm using translator ;)

taazz

  • Hero Member
  • *****
  • Posts: 5368
Re: Thread Safety?
« Reply #14 on: February 21, 2018, 07:23:21 am »
sorry I do not understand. As extra information, I never use Application.ProcessMessages not even in single thread mode. I never had the need for it.
OK. Graphic elements of the main thread have to be redrawn at that time while other work becomes in an additional thread. Otherwise the LCL elements will be "frozen".

Therefore I have to show additional SplashForm with animation from above of the main window. Or I have to redraw somehow LCL elements of the main window while the additional stream performs work. For this purpose I use Application.ProcessMessages.

If it isn't correct, then I will ask to set the "correct" example without application of ProcessMessages  :-[
if the splash screen has no action controls (buttons, edits etc) then things might be safe from re entrance problems if there is no click events or other code inside then its probably ok. For the record usually I do it like this.

Code: [Select]
  SplashScreen.create;
  BeginThreadCode(SplashScreen);

in thread code something like this.

Code: [Select]
TmyThread.execute;
begin
  QueueMethod(splashScreen.showmodal);//This is can be anywhere
  While not Terminated do begin
    run your code;
    QueueMethod(splashscreen.Update, UpdateData);
  end;
  QueueMethod(splashScreen.Finished,nil);
end;
not to often once or twice a second is enough in most times.
Code: [Select]
splashscreen.Update(Data:pointer);
begin
  Set visual controls to the new process.
end;

splashscreen.Finished(data:Pointer);
begin
  free; or what is needed.
end;

no processmessages is needed.
« Last Edit: February 21, 2018, 07:25:07 am by taazz »
Good judgement is the result of experience … Experience is the result of bad judgement.

OS : Windows 7 64 bit
Laz: Lazarus 1.4.4 FPC 2.6.4 i386-win32-win32/win64

 

TinyPortal © 2005-2018