TListView filled by Thread = Freeze

[FabDev 8]

Hello,

 

Following this sample :

https://blog.marcocantu.com/blog/2014_may_background_delphi_android_threads.html

 

I try to insert items in a TlistView using a Thread (see in attachment).

 

procedure TForm1.Button2Click(Sender: TObject);
begin

  AniIndicator1.Visible := True;
  AniIndicator1.Enabled := True;
  ListView1.enabled:=false;
  ListView1.beginupdate;
  FMyThreadTerminated:=false;
  FMyThread:=TThread.CreateAnonymousThread(procedure ()
  var
    I: Integer;
    Total: Integer;
  begin
    Total := 0;
    for I := 1 to MaxValue do
    begin
    //  if (I * 10 mod MaxValue) = 0 then
        TThread.Synchronize (TThread.CurrentThread,
          procedure ()
          begin
            ListView1.Items.Add.Text := 'Th: ' + I.ToString;
            Button2.text:= i.tostring;

            // On windows this is needed to update AniIndicator
           {$IFDEF MSWINDOWS}
             if GetQueueStatus(QS_ALLINPUT) <> 0 then
                Application.ProcessMessages;
           {$ENDIF}


          end);


        if application.Terminated then
        begin
           break;
        end;
    end;

    TThread.Synchronize (TThread.CurrentThread,
      procedure ()
      begin
        ListView1.Items.Add.Text := 'Thread: ' + Total.ToString;
        NotifyComplete;
      end);
  end);

  // FMyThread.FreeOnTerminate:=false;
 // FMyThread.OnTerminate:=ThreadTerminated;

  FMyThread.FreeOnTerminate:=false;

  FMyThread.Start;


end;

 

It's work fine on Windows and IOS except if you quit the application while thread (To insert item) is running : Application freeze !

I use :

 

procedure TForm1.FormClose(Sender: TObject; var Action: TCloseAction);
begin
     STopThread;
end;

procedure TForm1.StopThread;
begin
   if Assigned(FMyThread) then
  begin
    FMyThread.Terminate;
    WaitForSingleObject(FMyThread.handle,0);
   // FMyThread.WaitFor;
    //TerminateThread(FMyThread.Handle,0);   access violation
  end;
  FMyThread.Free;

end;

 

What is wrong in my code ?

I use Delphi 10.4.2.

 

 

TListView-Thread.zip

Edited July 16, 2021 by FabDev

[David Heffernan 2345]

Why do this in a thread? Seems like much complexity for no possible gain. 

[FabDev 8]

1 minute ago, David Heffernan said:

Why do this in a thread? Seems like much complexity for no possible gain.  

 

To not freeze application while inserting some records. Imagine record extracted from a query or a webservice.

What is more while it's working it's display a TAniIndicator...

[Dalija Prasnikar 1396]

You have a deadlock. 

 

Waiting for background thread in main thread that uses Synchronize is recipe for disaster. Namely, when you start waiting you are blocking main thread until thread is finished, but when thread encounters Synchronize it will try to execute that code in the context of the main thread, but since main thread is on hold at that time Synchronize will never be able to run and finish. 

 

You should use TThread.Queue instead.

[FabDev 8]

42 minutes ago, Dalija Prasnikar said:

You should use TThread.Queue instead.

 

Thank you Dalija, now it's (seems ?) work fine like this :

procedure TForm1.Button2Click(Sender: TObject);
begin

  AniIndicator1.Visible := True;
  AniIndicator1.Enabled := True;
  ListView1.enabled:=false;
  ListView1.beginupdate;
  FMyThreadTerminated:=false;

  TThread.queue(TThread.Current, procedure ()
  var
    I: Integer;
    Total: Integer;
  begin
    Total := 0;
    for I := 1 to MaxValue do
    begin

          TThread.Synchronize (TThread.CurrentThread,
          procedure ()
          begin
            ListView1.Items.Add.Text := 'Th: ' + I.ToString;
            Button2.text:= i.tostring;

            // On windows this is needed to update AniIndicator
           {$IFDEF MSWINDOWS}
             if GetQueueStatus(QS_ALLINPUT) <> 0 then
                Application.ProcessMessages;
           {$ENDIF}


          end);


        if application.Terminated then
        begin
           break;
        end;
    end;

    TThread.Synchronize (TThread.CurrentThread,
      procedure ()
      begin
        ListView1.Items.Add.Text := 'Thread: ' + Total.ToString;
        NotifyComplete;
      end);
  end);




end;

 

 

[Dalija Prasnikar 1396]

Now you have completely removed threads. I meant like this:  in places where you are calling TThread.Synchronize you should use TThread.Queue 

 

  FMyThread:=TThread.CreateAnonymousThread(procedure ()
  begin
    ...
        TThread.Queue(TThread.CurrentThread,
          procedure ()
          begin
          ...
          end);

     ...
    end;

    TThread.Queue(TThread.CurrentThread,
      procedure ()
      begin
     ...
      end);
  end);

[FabDev 8]

 

52 minutes ago, Dalija Prasnikar said:

you are calling TThread.Synchronize you should use TThread.Queue

 

It's work but now to have my TAniIndicator and button2.text changing I have had to call Application.ProcessMessages on each platform (Windows, IOS, MacOs...) :

procedure TForm1.Button2Click(Sender: TObject);
begin

  AniIndicator1.Visible := True;
  AniIndicator1.Enabled := True;
  ListView1.enabled:=false;
  ListView1.beginupdate;
  FMyThreadTerminated:=false;
  FMyThread:=TThread.CreateAnonymousThread(procedure ()
  begin
   TThread.queue (TThread.Current,
      procedure ()
      var
        I: Integer;
      begin
            for I := 1 to MaxValue do
            begin
                  ListView1.Items.Add.Text := 'Th: ' + I.ToString;
                  Button2.text:= i.tostring;
                  // On windows this is needed to update AniIndicator
                 {$IFDEF MSWINDOWS}
                    if GetQueueStatus(QS_ALLINPUT) <> 0 then
                 {$ENDIF}
                 begin
                        Application.ProcessMessages;
                 end;


               if application.Terminated then
                begin
                   break;
                end;
             end;
      end
    );

    TThread.queue (TThread.CurrentThread,
      procedure ()
      begin
        NotifyComplete;
      end);
  end);

  FMyThread.FreeOnTerminate:=false;

  FMyThread.Start;


end;

 

 

[Dalija Prasnikar 1396]

You have problems because you are not changing your original code with just replacing Synchronize with Queue.

 

In previous code you had Synchronize inside loop and now you have loop inside Queue. Go back to your original code and just use Queue instead of Synchronize. 

[Remy Lebeau 1397]

3 hours ago, Dalija Prasnikar said:

Waiting for background thread in main thread that uses Synchronize is recipe for disaster. Namely, when you start waiting you are blocking main thread until thread is finished, but when thread encounters Synchronize it will try to execute that code in the context of the main thread, but since main thread is on hold at that time Synchronize will never be able to run and finish. 

It is OK to wait on the thread if you use TThread.WaitFor() instead of WaitForSingleObject() directly.  When TThread.WaitFor() is called in the main thread, it dispatches pending TThread.Synchronize()/TThread.Queue() requests while waiting for the thread to terminate.  Just don't use TThread.WaitFor() with TThread.FreeOnTerminate=True, or else TThread.WaitFor() will crash when the thread actually terminates.

 

If you do use WaitForSingleObject() directly, you can call it in a loop with a timeout so that you can call CheckSynchronize() periodically.  Or, you can use (Msg)WaitForMultipleObjects() instead, waiting on both the thread and the global SyncEvent (and the message queue) at the same time.

3 hours ago, Dalija Prasnikar said:

You should use TThread.Queue instead.

In this particular example, yes, since the thread does not depend on the results of the sync requests.  But that is not always the case, sometimes Synchronize() is actually needed.

[Remy Lebeau 1397]

2 hours ago, Dalija Prasnikar said:

 

        TThread.Queue(TThread.CurrentThread,
          procedure ()
          begin
          ...
          end);

Note that passing a TThread object in the 1st parameter of TThread.Queue() will associate the request with that thread.  If the thread terminates before the request is processed by the main thread, the request will be cancelled.  The reason for this is to handle cases where a request accesses data/objects that exist inside the thread, you don't want the thread termination to invalidate that data before the request can use it.  But, if the request does not depend on the thread, ie the anonymous procedure is able to capture everything it needs to run independantly, then you should set the 1st parameter to nil instead.

[FabDev 8]

1 hour ago, Dalija Prasnikar said:

In previous code you had Synchronize inside loop and now you have loop inside Queue. Go back to your original code and just use Queue instead of Synchronize.  

You means that :

procedure TForm1.Button2Click(Sender: TObject);
begin

  AniIndicator1.Visible := True;
  AniIndicator1.Enabled := True;
  ListView1.enabled:=false;
  ListView1.beginupdate;
  FMyThreadTerminated:=false;
  FMyThread:=TThread.CreateAnonymousThread(procedure ()
  //TThread.queue(TThread.Current, procedure ()
   // Total: Integer;
 var
    I: Integer;
  begin
            for I := 1 to MaxValue do
            begin
               TThread.queue (TThread.Current,
                  procedure ()
                  begin
                              ListView1.Items.Add.Text := 'Th: ' + I.ToString;
                              Button2.text:= i.tostring;
                              // On windows this is needed to update AniIndicator
                             {$IFDEF MSWINDOWS}
                                if (GetQueueStatus(QS_ALLINPUT) <> 0)  then
                             {$ENDIF}
                             begin
                               //     Application.ProcessMessages;
                             end;


                  end
                );
                if application.Terminated then
                begin
                    break;
                end;
             end;

    TThread.queue (nil,
      procedure ()
      begin
        NotifyComplete;
      end);
  end);

  FMyThread.FreeOnTerminate:=false;

  FMyThread.Start;


end;

But it's doesn't work !

 

 

But this work fine excepted my TAniIndicator on Windows which work only with a Application.processMessage, but it's work fine on IOS :

 

procedure TForm1.Button2Click(Sender: TObject);
begin

  AniIndicator1.Visible := True;
  AniIndicator1.Enabled := True;
  ListView1.enabled:=false;
  ListView1.beginupdate;
  FMyThreadTerminated:=false;
  FMyThread:=TThread.CreateAnonymousThread(procedure ()
  //TThread.queue(TThread.Current, procedure ()
   // Total: Integer;
 var
    I: Integer;
  begin
            for I := 1 to MaxValue do
            begin
               TThread.Synchronize(TThread.Current,
                  procedure ()
                  begin
                              ListView1.Items.Add.Text := 'Th: ' + I.ToString;
                              Button2.text:= i.tostring;
                              // On windows this is needed to update AniIndicator
                             {$IFDEF MSWINDOWS}
                                if (GetQueueStatus(QS_ALLINPUT) <> 0)  then
                             {$ENDIF}
                             begin
                               //     Application.ProcessMessages;
                             end;


                  end
                );
                if application.Terminated then
                begin
                    break;
                end;
             end;

    TThread.queue (nil,
      procedure ()
      begin
        NotifyComplete;
      end);
  end);

  FMyThread.FreeOnTerminate:=false;

  FMyThread.Start;


end;

 

Edited July 16, 2021 by FabDev

[Remy Lebeau 1397]

1 hour ago, FabDev said:

You means that :

Yes.

1 hour ago, FabDev said:

But it's doesn't work !

Get rid of the use of TThread.CurrentThread when calling TThread.Queue(), pass nil instead, like I described earlier.  Chances are that the thread is terminating before the Queue() requests are processed by the main thread, so they are getting cancelled.

 

I would also suggest getting rid of the final call to TThread.Queue() after the loop, use the thread's OnTerminate event instead.

 

Try this:

procedure TForm1.FormClose(Sender: TObject; var Action: TCloseAction);
begin
  StopThread;
end;

procedure TForm1.StopThread;
begin
  if Assigned(FMyThread) then
  begin
    FMyThread.Terminate;
    FMyThread.WaitFor;
  end;
end;

procedure TForm1.Button2Click(Sender: TObject);
begin
  FMyThread := TThread.CreateAnonymousThread(
    procedure
    var
      I: Integer;
    begin
      for I := 1 to MaxValue do
      begin
        TThread.Queue (nil,
          procedure
          begin
            ListView1.Items.Add.Text := 'Th: ' + I.ToString;
            Button2.Text := I.ToString;
          end
        );
        if TThread.CurrentThread.Terminated or Application.Terminated then
          Exit;
      end;
    end
  );

  FMyThread.FreeOnTerminate := False;
  FMyThread.OnTerminate := ThreadFinished;
  FMyThread.Start;

  AniIndicator1.Visible := True;
  AniIndicator1.Enabled := True;
  ListView1.Enabled := False;
  ListView1.BeginUpdate;
end;

procedure TForm1.ThreadFinished(Sender: TObject);
begin
  TThread.ForceQueue(nil, FMyThread.Free);
  FMyThread := nil;
  NotifyComplete;
end;

procedure TForm1.NotifyComplete;
begin
  ListView1.EndUpdate;
  ListView1.Enabled := True;
  AniIndicator1.Enabled := False;
  AniIndicator1.Visible := False;
end;

 

[Dalija Prasnikar 1396]

Animation is not working because thread is doing very little work besides queuing work to the main thread. This gives very little chance to the main thread to do anything besides processing the queued work and repaint animation. This is matter of timing so it is not surprising that you have different behavior on different platforms. 

 

You can either use ProcessMessages to force main thread message processing and repainting the animation, or you can put thread to sleep for short period of time. However, sleeping will slow down the whole process, so you don't want to call it at every iteration.

 

Following code works without issues.

 

There are some additional notes, some already mentioned by @Remy Lebeau

 

First, I have changed StopThread procedure to just call FreeAndNil(FMyThread) - freeing the thread will also call Terminate and WaitFor - this is why it causes the deadlock is you are using Synchronize. Niling the thread is there so you can click the button again otherwise you would leak the thread.

If the thread is already running you will not be able to start the thread again, but you will be able to do so after the first thread is finished. NotifyComplete procedure will also call StopThread to release the thread because it makes no sense to keep dead thread around. 

 

Calling TThread.Queue(TThread.CurrentThread will discard any queued events after the thread is dead. In this example it is important to do so, because form is closing any we don't want to run any already queued events after form is released as this would cause AV. If there is something that needs to be always executed then such code should be in OnTerminate event. When it comes to anonymous method variable capture, they will be captured and their lifetime will be extended, but that only means variable will be accessible, but does not guarantee that the content (object) will be alive, you can only safely use variables of value types and managed types. For others, you need to be sure that they will be alive as long as anonymous method is running. In this case ListView1 and Button2 will not be alive when the form is closed.

 

 

procedure TForm1.FormClose(Sender: TObject; var Action: TCloseAction);
begin
  StopThread;
end;

procedure TForm1.StopThread;
begin
  FreeAndNil(FMyThread);
end;

procedure TForm1.NotifyComplete;
begin
  AniIndicator1.Visible := False;
  AniIndicator1.Enabled := False;
  ListView1.EndUpdate;
  ListView1.Enabled := true;
  StopThread;
end;

procedure TForm1.Button2Click(Sender: TObject);
begin
  if Assigned(FMyThread) then
    Exit;
  AniIndicator1.Visible := True;
  AniIndicator1.Enabled := True;
  ListView1.Enabled:=false;
  ListView1.BeginUpdate;
  FMyThread:=TThread.CreateAnonymousThread(procedure ()
  var
    I: Integer;
    Total: Integer;
  begin
    Total := 0;
    for I := 1 to MaxValue do
    begin
        TThread.Queue(TThread.CurrentThread,
          procedure ()
          begin
            ListView1.Items.Add.Text := 'Th: ' + I.ToString;
            Button2.text:= i.tostring;
          end);

        if Application.Terminated then
        begin
          break;
        end
        else
          begin
            if i mod 10 = 0 then
              Sleep(1);
          end;
    end;

    TThread.Queue(TThread.CurrentThread,
      procedure()
      begin
        ListView1.Items.Add.Text := 'Thread: ' + Total.ToString;
        NotifyComplete;
      end);
  end);
  FMyThread.FreeOnTerminate := False;
  FMyThread.Start;
end;

 

[Lars Fosdal 1792]

I usually do this as a two pass operation.  I first start a thread that fills a memory structure.

Once the thread is done, I trigger an update in the main thread that fills the UI from the memory structure.

I have nothing but negative experiences with doing UI updates directly from threads.

[Remy Lebeau 1397]

On 7/17/2021 at 1:03 AM, Dalija Prasnikar said:

First, I have changed StopThread procedure to just call FreeAndNil(FMyThread) - freeing the thread will also call Terminate and WaitFor - this is why it causes the deadlock is you are using Synchronize.

I always advise people NOT to rely on the TThread destructor terminating and waiting on the running thread, as doing so has been the source of many headaches in the past.  Terminating and waiting on the running thread should always be an explicit operation before destroying the TThread object, IMHO.

[Dalija Prasnikar 1396]

2 hours ago, Remy Lebeau said:

I always advise people NOT to rely on the TThread destructor terminating and waiting on the running thread, as doing so has been the source of many headaches in the past.  Terminating and waiting on the running thread should always be an explicit operation before destroying the TThread object, IMHO.

Maybe this has been source of trouble in the past, but TThread destructor terminates thread and waits for it at least since Delphi 7. 

 

Under some circumstances code might require explicit termination and waiting before calling destructor. For instance if destructor is overridden and can destroy fields that are used by the thread, but if the order of destruction is important, I would still prefer code that handles proper shutdown sequence inside such overridden TThread class than handling it from the outside code. When it is handled from the outside there is always greater possibility that someone will forget to explicitly terminate and wait.

[Remy Lebeau 1397]

2 hours ago, Dalija Prasnikar said:

Maybe this has been source of trouble in the past, but TThread destructor terminates thread and waits for it at least since Delphi 7. 

Yes, it has done so for a very long time, since at least Delphi 5, probably even since TThread was first introduced.  But just because it CAN doesn't mean it SHOULD.  And like I have said, many people have had problems related to this "feature" over the years, which is why I never recommend relying on it.

[FabDev 8]

On 7/17/2021 at 10:03 AM, Dalija Prasnikar said:

Following code works without issues.

 

Thank you very much Dalija, your version work fine (tested on Windows and Android) .

 

Thank you to all other for interesting answer !