Not Threadsafe??

[Ian Branch 128]

Hi Team,

 

I have used the following construct multiple times for a long time..  Currently D11.3 but used in older Delphis.

    //
    Thread := TThread.CreateAnonymousThread(
      procedure
      begin
        //
        DBSBackup.Execute(Some SQL code);
        //
      end);
    try
      Thread.FreeOnTerminate := False;
      H := Thread.Handle;
      Thread.Start;
      while MsgWaitForMultipleObjects(1, H, False, INFINITE, QS_ALLINPUT) = (WAIT_OBJECT_0 + 1) do
        Application.ProcessMessages;
    finally
      FreeAndNil(Thread);
    end;
    //

DBSBackup is an ElevateDB TEDBSession.

Whilst this is happening an activity indicator is spinning around on the screen so the User knows the App is alive and still doing something.

I have been advised that 'Application.ProcessMessages' is not thread safe.

Is this the case?  If so, what is a better way to achieve the above?

 

 

Regards & TIA,

Ian

[programmerdelphi2k 237]

you must understand that a "Thread" is like "another software within your software" (an Aliens) 🙂, that is, you need to deal with it as if you were always checking what the "other software is doing at this moment within the your software".

Therefore, it is always necessary not to mix background tasks (the thread) with foreground tasks (your app).

Therefore, when it is necessary to update the interface, you are obliged to ask for some "synchronization with the main thread". It's like an arm wrestling match... and we hope we always win, otherwise chaos will ensue...

 

When you use "Application.ProcessMessages", you are forcing the application to always pay special attention to its task, regardless of what it is doing at the moment. And, in the case of using the database, we must take into account that, after you send a command to the database, you are delegating a task to it, and therefore, you must hope that everything goes well there, otherwise , you will get an exception that will propagate through your background thread, which will affect the functioning of your main thread (your app).

 

So, while nothing really serious happens, it doesn't mean your code is 100%. But, simply, that he still hasn't found something so serious that it could stop everything!

 

Creating an anonymous thread is not necessarily creating a multi-parallel task! A background thread is just a background thread!
Multi-parallelism is when many threads work in a way that, in human perception, and, moreover, use multiple CPUs simultaneously.

 

maybe help you if reading the FireDAC way in https://docwiki.embarcadero.com/RADStudio/Alexandria/en/Multithreading_(FireDAC)

• basically, the "magic" happens on TFDManager (all FDConnection have it, same that you dont create it)
  • it do the "pooling service control", a semaphore to the connections...
  • the hierachy in FireDAC is:  TFDManager -> TFDConnection -> TFD_datasets_objects, then, always exists 1 FDManager!
• It is the FDManager who is responsible for sharing the connections with the database, in order to avoid the bottlenecks of initializing the connection itself.

Realize that each thread must create its own database access objects, however, it is the FDManager who determines which connections will be used, and by who is registered in it. There will be only one FDManager, which shares its information with every FDConnection you create. However, only in connections of the type "Private ( FDConnection created in each thread)" or "Persistent (defined in the FireDAC FDConnectionDefs.ini file) "

 

Now, try to see the differences between your code and the one used in FireDAC...

Edited March 24, 2023 by programmerdelphi2k

[programmerdelphi2k 237]

here an Elevante article about MultiThreading and DB sessions...

https://www.elevatesoft.com/manual?action=viewtopic&id=dbisam4&product=delphi&version=5&topic=multi_threaded_applications

[Anders Melander 1815]

2 hours ago, Ian Branch said:

H := Thread.Handle;
Thread.Start;
while MsgWaitForMultipleObjects(1, H, False, INFINITE, QS_ALLINPUT) = (WAIT_OBJECT_0 + 1) do
  Application.ProcessMessages;

 

I'm guessing the purpose of that code block is to keep the application responsive while the tread is executing, right?

 

ProcessMessages is very rarely the solution - for anything. It's fine in quick and dirty, test code, but it doesn't belong in production code.

While there is no problem with ProcessMessages in regard to threads in your code (you're not calling it from a thread), it often makes your UI vulnerable to undesired reentrancy. For example, let's say you have a button on a form with the following OnClick handler:

procedure TMyForm.MyButtonClick(Sender: TObject);
begin
  // Increment a counter to show we're busy
  Tag := Tag + 1;
  Caption := IntToStr(Tag);
  
  // Emulate a task that takes a long time
  for var i := 0 to 1000 do
  begin
    // Busy, busy...
    Sleep(100);
    // ...But stay "reponsive"...
    Application.ProcessMessages;
  end;

  // We're done; Decrement the counter
  Tag := Tag - 1;
  Caption := IntToStr(Tag);
end;

The problem here is that unless you take extra steps to prevent it, the user can just keep pressing the button (you'll see the counter keeps incrementing); You've created a recursion through ProcessMessages.

The easiest way to prevent that is the disable the UI (e.g. Form.Enabled := False), but the best way is to not use ProcessMessages in the first place.

Have you tried pressing the Windows close button (or just [Alt]+[F4]) while that code is executing? I'll bet that doesn't end well.

 

You can create a message loop that processes selected messages instead. For example WM_PAINT and a few others are safe to handle.
Here's one I've used in an application I worked on. It is called while doing work that updates a progress bar (with an Abort button). There's a bunch of stuff that only makes in that particular application, but I'm sure you get the meaning.

type
  TFormRedacted = class(TForm)
  private
    FProgressStart: TStopWatch;
    FProgressThrottle: TStopWatch;
    FLastMessagePump: TStopwatch;
    FProgressEnableAbort: boolean;
    FProgressAborted: boolean;
  ...
  end;

procedure TFormRedacted.ProcessProgressMessages(Force: boolean);
var
  Msg: TMsg;
begin
  if (not Force) and (FLastMessagePump.IsRunning) and (FLastMessagePump.ElapsedMilliseconds < MessagePumpRate * ProgressUpdateFactor) then
    exit;

  FLastMessagePump.Reset;

  var ProgressHandle := ButtonProgressAbort.Handle;

  // Indicate to madExcept freeze detection that we're not frozen
  FreezeDetection.IndicateApplicationNotFrozen;

  try
    // Allow threads to synchronize to avoid deadlock (e.g. busy loop showing progress waiting for thread to complete (e.g. spell check dictionary load)).
    CheckSynchronize;

    Msg.message := 0;
    try

      // Look for Escape key pressed.
      if (FProgressEnableAbort) and (Application.Active) and (GetAsyncKeyState(VK_ESCAPE) <> 0) then
      begin
        // Wait for Escape key to be released again.
        while (GetAsyncKeyState(VK_ESCAPE) <> 0) do
          Sleep(1);

        // Clear message queue of keyboard messages
        while (PeekMessage(Msg, 0, WM_KEYFIRST, WM_KEYLAST, PM_REMOVE)) do
        begin
          if (Msg.message = WM_QUIT) then
            exit;
        end;

        PromptAbortProgress;

        // Wait for Escape key to be released so dismissing the abort prompt
        // dialog with escape doesn't trigger a new prompt.
        while (GetAsyncKeyState(VK_ESCAPE) <> 0) do
          Sleep(1);

        // Clear message queue of keyboard messages
        while (PeekMessage(Msg, 0, WM_KEYFIRST, WM_KEYLAST, PM_REMOVE)) do
        begin
          if (Msg.message = WM_QUIT) then
            exit;
        end;
      end;

      // Process mouse move for all windows so hot tracking works.
      // Don't process mouse movement if a mouse key is down.
      // This tries to avoid recursions caused by scrollbar movement causing work that
      // ends up calling this method.
      while (PeekMessage(Msg, 0, WM_MOUSEMOVE, WM_MOUSEMOVE, PM_NOREMOVE)) and (GetKeyState(VK_LBUTTON) = 0) do
      begin
        PeekMessage(Msg, 0, WM_MOUSEMOVE, WM_MOUSEMOVE, PM_REMOVE);

        if (Msg.message = WM_QUIT) then
          exit;

        DispatchMessage(Msg);
      end;

      // Process mouse hover/enter/exit messages for all windows so button state will be updated
      while (PeekMessage(Msg, 0, WM_NCMOUSEHOVER, WM_MOUSELEAVE, PM_REMOVE)) do
      begin
        if (Msg.message = WM_QUIT) then
          exit;

        DispatchMessage(Msg);
      end;

      // Process timer message for all windows so animation works - THIS IS DANGEROUS SINCE ALL TIMERS WILL BE PROCESSED
      while (PeekMessage(Msg, 0, WM_TIMER, WM_TIMER, PM_REMOVE)) do
      begin
        if (Msg.message = WM_QUIT) then
          exit;

        DispatchMessage(Msg);
      end;

      // Process mouse messages for button so user can press Stop button
      while (PeekMessage(Msg, ProgressHandle, WM_MOUSEFIRST, WM_MOUSELAST, PM_REMOVE)) do
      begin
        if (Msg.message = WM_QUIT) then
          exit;

        DispatchMessage(Msg);
      end;

      // Process cursor update messages for all windows so cursor stays responsive
      while (PeekMessage(Msg, 0, WM_SETCURSOR, WM_SETCURSOR, PM_REMOVE)) do
      begin
        if (Msg.message = WM_QUIT) then
          exit;

        DispatchMessage(Msg);
      end;

      // Process progress bar messages - This includes WM_TIMER and WM_PAINT used for progress bar animation
      while PeekMessage(Msg, Progress.Handle, 0, 0, PM_REMOVE) do
      begin
        if (Msg.message = WM_QUIT) then
          exit;

        DispatchMessage(Msg);
      end;

      // Process paint messages for all windows so UI can repaint itself
      while PeekMessage(Msg, 0, WM_PAINT, WM_PAINT, PM_REMOVE) or
        PeekMessage(Msg, 0, WM_ERASEBKGND, WM_ERASEBKGND, PM_REMOVE) or
        PeekMessage(Msg, Handle, DXM_SKINS_POSTREDRAW, DXM_SKINS_POSTREDRAW, PM_REMOVE) or
        PeekMessage(Msg, 0, WM_PRINT, WM_PRINT, PM_REMOVE) or
        PeekMessage(Msg, 0, WM_PRINTCLIENT, WM_PRINTCLIENT, PM_REMOVE) do
      begin
        if (Msg.message = WM_QUIT) then
          exit;

        DispatchMessage(Msg);
      end;

      PeekMessage(Msg, 0, WM_NULL, WM_NULL, PM_NOREMOVE); // Avoid window ghosting due to unresponsiveness on Vista+

    finally
      if (Msg.message = WM_QUIT) then
      begin
        PostQuitMessage(Msg.wParam);
        FProgressAborted := True;
      end;
    end;
  finally
    FLastMessagePump.Start;
  end;

  if (FProgressAborted) then
    AbortProgress;
end;

Here's another version of it: https://bitbucket.org/anders_melander/better-translation-manager/src/f96e7dcdba22667560178d32aebb5137484107f0/Source/amProgress.pas#lines-444

[Ian Branch 128]

All,

Thank you very much for your inciteful and informative inputs.  Very much appreciated.

I put Threads in the same category as Pointers - A black art.  Something I never fully grasped, but happy to use if it achieves an end.

[Anders Melander 1815]

1 minute ago, Ian Branch said:

Something I never fully grasped, but happy to use if it achieves an end.

Yes, if your end is to make a mess of things, then threads will work wonders for you.

[Lars Fosdal 1793]

Threads are not hard, as long as you play by the rules as described by @Dalija Prasnikar 

There is also good advice in OmniThreadLibrary by @Primož Gabrijelčič

 

[Fr0sT.Brutal 900]

Just launch the thread and let it work, catch the event when it's done.

[Dalija Prasnikar 1405]

11 hours ago, Ian Branch said:

I have been advised that 'Application.ProcessMessages' is not thread safe.

Is this the case?  If so, what is a better way to achieve the above?

Application.ProcessMessages in not thread-safe means you cannot call it from the background thread. Since you are calling it from main thread that alone is not a problem in your code.

 

However, one of the points of using threads is to keep application responsive to messages so you don't have to use Application.ProcessMessages which is not the best practice for many reasons.

 

There are many ways for handling threads, and which one you choose depends on what particular thread does and the application logic. Since your code actually waits until the operation completes, then the simplest code that would achieve what you want with threads would be something like this:

 

  TMainForm = class(TForm)
  private
    Processing: Boolean;
    procedure EnableUI;
    procedure DisableUI;
    ...
  end;

procedure TMainForm.BtnClick(Sender: TObject);
begin
  if Processing then
    Exit;
  Processing := True;
  try
    DisableUI;
    TThread.CreateAnonymousThread(
      procedure
      begin
        try
          // do work here
        finally
          TThread.Queue(nil,
            procedure
            begin
              EnableUI;
              Processing := False;
            end);
        end;
      end).Start;
  except
    EnableUI;
    Processing := False;
  end;
end;

procedure TMainForm.FormCloseQuery(Sender: TObject; var CanClose: Boolean);
begin
  CanClose := not Processing;
end;

 

In EnableUI and DisableUI methods you can put whatever code you need to show the user that you are doing some work or disable/enable some actions. The rest is pretty simple - if user tries to start processing again, code will not let it do that (that is something you should have even when you use Application.ProcessMessages as it allows reentrancy and user could start same operation while previous one is not finished). When thread is done (regardless of whether it completes successfully or not), it will enable UI (this needs to be called from the main thread and this is why there is TThread.Queue (or you can use TThread.Synchronize here - the main difference is that Queue is non blocking call and Synchronize is blocking). You can also catch exceptions and show message to user in similar manner. Thread will self-destruct after it is finished so you don't have to worry about its memory management.

 

To avoid complicated cleanup and potentially breaking unfinished work thread is doing, the simplest way is to prevent user from closing form while thread is working in FormCloseQuery event. 

 

Note: Running DBSBackup.Execute opens the question about thread-safety of that code - in other words whether main thread can use DBSBackup instance and anything directly related while thread is running - which would not be thread-safe to do. But that part is another story.

 

Edited March 24, 2023 by Dalija Prasnikar

[Pat Foley 52]

4 hours ago, Dalija Prasnikar said:

procedure TMainForm.FormCloseQuery(Sender: TObject; var CanClose: Boolean); begin CanClose := not Processing; end;

I like that I have used just a showmessage when CanClose is false;

[programmerdelphi2k 237]

the use of variables to determine flow control, as used above, can lead to the false security of creating new threads/execution in the same unit, since any other "thread in the same unit" could change the value of these variables, allowing chaos is installed.

[Dalija Prasnikar 1405]

17 minutes ago, programmerdelphi2k said:

the use of variables to determine flow control, as used above, can lead to the false security of creating new threads/execution in the same unit, since any other "thread in the same unit" could change the value of these variables, allowing chaos is installed.

While more complicated scenario with multiple threads could require more sophisticated code, this approach would work without issues even if there are multiple tasks that require running in background thread. There would be no chaos because only single thread can run at the time. This is what "if Processing then Exit" is here for. User will not be able to start another thread as long as some thread is already running. Only when thread finishes, new thread can be started.

[Dalija Prasnikar 1405]

29 minutes ago, Pat Foley said:

I like that I have used just a showmessage when CanClose is false;

Yes, it is a good idea to show user the reason why form cannot be closed.

[programmerdelphi2k 237]

29 minutes ago, Dalija Prasnikar said:

There would be no chaos because only single thread can run at the time

... if only one, then main-thread it's enough.

[Anders Melander 1815]

1 hour ago, programmerdelphi2k said:

... if only one, then main-thread it's enough.

I think he would like for the application not to appear "hung" while it's waiting for the database task to complete.

[programmerdelphi2k 237]

6 minutes ago, Anders Melander said:

not to appear "hung" while it's waiting

you're right! 👍

[Remy Lebeau 1436]

10 hours ago, Dalija Prasnikar said:

Since your code actually waits until the operation completes, then the simplest code that would achieve what you want with threads would be something like this:

 

  TMainForm = class(TForm)
  private
    Processing: Boolean;
    procedure EnableUI;
    procedure DisableUI;
    ...
  end;

procedure TMainForm.BtnClick(Sender: TObject);
begin
  if Processing then
    Exit;
  Processing := True;
  try
    DisableUI;
    TThread.CreateAnonymousThread(
      procedure
      begin
        try
          // do work here
        finally
          TThread.Queue(nil,
            procedure
            begin
              EnableUI;
              Processing := False;
            end);
        end;
      end).Start;
  except
    EnableUI;
    Processing := False;
  end;
end;

procedure TMainForm.FormCloseQuery(Sender: TObject; var CanClose: Boolean);
begin
  CanClose := not Processing;
end;

 

In that kind of example, I would probably opt to use the TThread.OnTerminate event instead, eg:

type
  TMainForm = class(TForm)
  private
    ProcessingThread: TThread;
    procedure EnableUI;
    procedure DisableUI;
    procedure ThreadFinished(Sender: TObject);
    procedure ThreadTerminated(Sender: TObject);
    ...
  end;

procedure TMainForm.BtnClick(Sender: TObject);
begin
  if Assigned(ProcessingThread) then
    Exit;
  DisableUI;
  try
    ProcessingThread := TThread.CreateAnonymousThread(
      procedure
      begin
        // do work here
      end
    );
    ProcessingThread.OnTerminate := ThreadFinished;
    try
      ProcessingThread.Start;
    except
      FreeAndNil(ProcessingThread);
      raise;
    end;
  except
    EnableUI;
  end;
end;

procedure TMainForm.ThreadFinished(Sender: TObject);
begin
  ProcessingThread := nil;
  EnableUI;
end;

procedure TMainForm.ThreadTerminated(Sender: TObject);
begin
  ProcessingThread := nil;
  Close;
end;

procedure TMainForm.FormCloseQuery(Sender: TObject;
  var CanClose: Boolean);
begin
  CanClose := not Assigned(ProcessingThread);
  if not CanClose then
  begin
    ProcessingThread.OnTerminate := ThreadTerminated;
    ProcessingThread.Terminate;
    // display a "Please wait" UI ...
  end;
end;

 

[Ian Branch 128]

@Dalija Prasnikar % @Remy Lebeau

 

I like both your solutions.

As was noted, att the App sits there until the process is finished.

I would like, if needed, for the User to be able to move the form aside, out of the road, on the screen, or even another screen, while the process is running, if possible.  Particularly on long processes.

 

Regards,

Ian

Edited March 24, 2023 by Ian Branch

[programmerdelphi2k 237]

hi @Ian Branch

 

try my sample here https://en.delphipraxis.net/topic/8653-vcl-or-fmx-my-sample-using-thread-running-in-another-forms-for-some-tasks/

[Remy Lebeau 1436]

3 hours ago, Ian Branch said:

As was noted, att the App sits there until the process is finished.

Blocking the app does not require blocking the UI thread.  Your code should not sit there blocking the main UI thread waiting until the worker thread ends.  Let the code in the UI thread return control back to the VCL's main message loop.  Have the thread notify your code when it has finished its work.  Simply prevent the user from interacting with your UI in the meantime, if that is what you need to do.  Both of the solutions presented to you above will accomplish that for you.

 

You should read the following article: Modality, part 1: UI-modality vs code-modality It is written from the perspective of the Win32 API, but since the VCL is based on Win32, the same concept applies in this case.

3 hours ago, Ian Branch said:

I would like, if needed, for the User to be able to move the form aside, out of the road, on the screen, or even another screen, while the process is running, if possible.

That requires message handling.  Of course, you will get that with an Application.ProcessMessages() loop, but that opens up your code to a host of issues, which have already been pointed out earlier.  If you are going to process messages anyway, you may as well let the VCL do it for you.  Don't block your UI thread.

[Dalija Prasnikar 1405]

14 hours ago, Ian Branch said:

As was noted, att the App sits there until the process is finished.

I would like, if needed, for the User to be able to move the form aside, out of the road, on the screen, or even another screen, while the process is running, if possible.  Particularly on long processes.

Like @Remy Lebeau already said, both his and mine solution will do that. Your UI will be responsive as VCL main message pump will keep running while your thread is doing work in the background.

 

If you are confused by DisableUI/EnableUI method names, code in those methods will not block that message pump either unless you put some infinite loop there on purpose. Both can be completely empty. You should put there code that will show/hide progress spinner or you can disable/enable some buttons so that user cannot start some other operation while backup is running.