Threads und StringList

[IMPEGA]

Ich versuche mich erneut etwas mit Threads zu beschäftigen.
Das Thema ist schon ein wenig kompliziert für mich. Ich habe inzwischen schon etwas zusammengebaut, habe aber dazu ein Paar bzw. eine wichtige Frage.
Welche Methode ist Threadsicher, oder eben besser. Vorausgesetzt, so kann ich es überhaupt aufbauen. Von dem ganzen Lesen bin ich schon total durcheinander.
Hier mein 2 Methoden, ich denke beide kann ich verwenden, leider kann ich aber nicht erkennen, welche davon besser ist.
Sorry für die möglichen Fehler, ich bin nur ein Hobby-Programmierer.
Hintergrund ist halt, eine sehr große Liste aus Datei einlesen, in mehreren Threads Daten abfragen. Nach sagen wir 30 Minuten, anhalten.
Danach einfach weiter die Liste abarbeiten. Bei Destroy kommt später noch Speichern der Datei. Bei erneutem Programmstart, wird die kleinere Datei genommen und weiter damit gearbeitet.
So in etwa soll es funktionieren. Ein ThreadPool in der Form kriege ich nicht hin, also versuche ich es so.
Es handelt sich um eine sehr große Datei, deshalb muss es in mehreren Etappen erledigt werden.

Meine Frage bezieht sich hauptsächlich auf die StringList im Thread und als Ergebnis.

1: Mein Favorit, weil ich sowohl String als auch StringList nutzen kann.

Delphi-Quellcode:

    procedure ButtonAbbruchClick(Sender: TObject);
    procedure ButtonEXITClick(Sender: TObject);
    procedure ButtonStartClick(Sender: TObject);
    procedure FormDestroy(Sender: TObject);
    procedure ButtonWeiterClick(Sender: TObject);
  private
    { Private-Deklarationen }
    CriticalSection  : TCriticalSection;
    UrlList          : TStringList;
    ErgebnisListe    : TStringList;
    ActiveThreads    : Int64;
    IdleJobs         : Int64;
    JobsFinished     : Int64;
    procedure UpdateLabelActiveThreads;
    procedure UpdateLabelJobs;
    procedure UpdateLabelFinished;
  public
    { Public-Deklarationen }
  end;

var
  Form1: TForm1;
  ShouldStop: Boolean = False;

implementation

{$R *.dfm}

type
  TURLCheckerThread = class(TThread)
  private
    FUrlList         : TStrings;
    FResultList      : TStrings;
    FCriticalSection : TCriticalSection;
    FOnStatus        : TProc<string>;
    FShouldStop      : PBoolean;
  protected
    procedure Execute; override;
    procedure UpdateStatus(const Msg: string);
  public
    constructor Create(UrlList: TStrings; CriticalSection: TCriticalSection; OnStatus: TProc<string>; ShouldStop: PBoolean; ResultList: TStrings);
  end;

{ TWorkerThread }
constructor TURLCheckerThread.Create(UrlList: TStrings; CriticalSection: TCriticalSection; OnStatus: TProc<string>; ShouldStop: PBoolean; ResultList: TStrings);
begin
  inherited Create(False);
  FUrlList         := UrlList;
  FResultList      := ResultList;
  FCriticalSection := CriticalSection;
  FOnStatus        := OnStatus;
  FShouldStop      := ShouldStop;
  FreeOnTerminate  := True;

  {Zähle aktive Threads}
  TThread.Synchronize(nil, procedure
    begin
      Inc(Form1.ActiveThreads);
      Form1.UpdateLabelActiveThreads;
    end);
end;

procedure TURLCheckerThread.UpdateStatus(const Msg: string);
begin
  if Assigned(FOnStatus) then
     TThread.Synchronize(TThread.Current, procedure begin FOnStatus(Msg); end);
end;

procedure TURLCheckerThread.Execute;
var
  Url  : string;
  Http : TIdHTTP;
begin
  Http := TIdHTTP.Create(nil);
    try
      while not Terminated and not FShouldStop^ do
        begin
          Url := '';
          FCriticalSection.Acquire;
            try
              if FUrlList.Count > 0 then
                begin
                  Url := FUrlList[0];
                  FUrlList.Delete(0);
                end;
            finally
              FCriticalSection.Release;
            end;

          if Url = '' then Break;

          try
            Sleep(Random(2000)); {Hier kommt später meine Aufgabe}
//          Http.Head(Url);

            FCriticalSection.Acquire;              {Keine Ahnung ob es richtig ist, habe aber rausgelesen dass man StringList auf jeden Fall mit CriticalSection absichern soll}
              try
                FResultList.Add('OK: ' + Url);     {Ergebnis in StringList eintragen}
                FResultList.Add('Zweite Zeile');   {Ergebnis in StringList eintragen}
                FResultList.Add('Dritte Zeile');   {Ergebnis in StringList eintragen}
              finally
                FCriticalSection.Release;
              end;

            UpdateStatus('OK: ' + Url);   {Damit kann ich zB. die Status ON oder OFF ausgeben, oder weglassen und NUR FResult als Ergebnis nutzen}
          except
            on E: Exception do UpdateStatus('FAIL: ' + Url + ' - ' + E.Message);
          end;

          TThread.Synchronize(nil, procedure
            begin
              Dec(Form1.IdleJobs);
              Form1.UpdateLabelJobs;

              Inc(Form1.JobsFinished);
              Form1.UpdateLabelFinished;
            end);
        end;
    finally
      Http.Free;

      TThread.Synchronize(nil, procedure
        begin
          Dec(Form1.ActiveThreads);
          Form1.UpdateLabelActiveThreads;
        end);
    end;
end;

procedure TForm1.FormDestroy(Sender: TObject);
begin
  FreeAndNil(CriticalSection);
  FreeAndNil(ErgebnisListe);
  FreeAndNil(UrlList);
end;

procedure TForm1.ButtonStartClick(Sender: TObject);
var
  Threads : array[0..99] of TURLCheckerThread;
  Index   : Integer;
begin
  if Assigned(CriticalSection) then FreeAndNil(CriticalSection); {Falls ich nach Cancel noch mal Start drücke statt Weiter}
  if Assigned(UrlList)         then FreeAndNil(UrlList);         {Falls ich nach Cancel noch mal Start drücke statt Weiter}

  UrlList         := TStringList.Create;
  CriticalSection := TCriticalSection.Create;
  ShouldStop      := False;

  for Index := 1 to 100 do
    begin
      URLList.Add('http://google.com/' + IntToStr(Index));
    end;

  LabelJobs.Caption := '100';
  LabelIdle.Caption := '100';
  IdleJobs          := 100;
  JobsFinished      := 0;

  ErgebnisListe := TStringList.Create;

  for Index := 1 to SpinEditThreads.Value do
    Threads[Index] := TURLCheckerThread.Create(UrlList, CriticalSection,
      procedure(Msg: string)
        begin
          RichEditLog.Lines.Add(Msg);
          RichEditLog.Lines.Add(ErgebnisListe.Text);
          ErgebnisListe.Clear;                      {Nicht vergessen, sonst wird die StringList immer weiter befüllt}
        end, @ShouldStop, ErgebnisListe);
end;

procedure TForm1.ButtonWeiterClick(Sender: TObject);
var
  Threads : array[0..99] of TURLCheckerThread;
  Index   : Integer;
begin
  ShouldStop := False;

  for Index := 1 to SpinEditThreads.Value do
    Threads[Index] := TURLCheckerThread.Create(UrlList, CriticalSection,
      procedure(Msg: string)
        begin
          RichEditLog.Lines.Add(Msg);                {Ergebnis als String}
          RichEditLog.Lines.Add(ErgebnisListe.Text); {Ergebnis als StringList}
          ErgebnisListe.Clear;                       {Sehr wichtig, StringList resetten}
        end, @ShouldStop, ErgebnisListe);
end;

procedure TForm1.ButtonAbbruchClick(Sender: TObject);
begin
  ShouldStop := True;
end;

procedure TForm1.UpdateLabelActiveThreads;
begin
  TThread.Synchronize(nil, procedure
    begin
      LabelActiveThreads.Caption := IntToStr(ActiveThreads);
    end);
end;

procedure TForm1.UpdateLabelJobs;
begin
  TThread.Synchronize(nil, procedure
    begin
      LabelIdle.Caption     := IntToStr(IdleJobs);
    end);
end;

procedure TForm1.UpdateLabelFinished;
begin
  TThread.Synchronize(nil, procedure
    begin
      LabelFinished.Caption := IntToStr(JobsFinished);
    end);
end;

procedure TForm1.ButtonEXITClick(Sender: TObject);
begin
  ShouldStop := True;
  Close;
end;

2:

Delphi-Quellcode:

    procedure ButtonAbbruchClick(Sender: TObject);
    procedure ButtonEXITClick(Sender: TObject);
    procedure ButtonStartClick(Sender: TObject);
    procedure FormDestroy(Sender: TObject);
    procedure ButtonWeiterClick(Sender: TObject);
  private
    { Private-Deklarationen }
    CriticalSection  : TCriticalSection;
    UrlList          : TStringList;
    ActiveThreads    : Int64;
    IdleJobs         : Int64;
    JobsFinished     : Int64;
    procedure UpdateLabelActiveThreads;
    procedure UpdateLabelJobs;
    procedure UpdateLabelFinished;
  public
    { Public-Deklarationen }
  end;

var
  Form1      : TForm1;
  ShouldStop : Boolean = False;

implementation

{$R *.dfm}

type
  TURLCheckerThread = class(TThread)
  private
    FUrlList         : TStrings;
    FCriticalSection : TCriticalSection;
    FOnStatus        : TProc<TStringList>;
    FShouldStop      : PBoolean;
  protected
    procedure Execute; override;
    procedure UpdateStatus(const Msg: TStringList);
  public
    constructor Create(UrlList: TStrings; CriticalSection: TCriticalSection; OnStatus: TProc<TStringList>; ShouldStop: PBoolean);
  end;

{ TWorkerThread }

constructor TURLCheckerThread.Create(UrlList: TStrings; CriticalSection: TCriticalSection; OnStatus: TProc<TStringList>; ShouldStop: PBoolean);
begin
  inherited Create(False);
  FUrlList         := UrlList;
  FCriticalSection := CriticalSection;
  FOnStatus        := OnStatus;
  FShouldStop      := ShouldStop;
  FreeOnTerminate  := True;

  {Zähle aktive Threads}
  TThread.Synchronize(nil, procedure
    begin
      Inc(Form1.ActiveThreads);
      Form1.UpdateLabelActiveThreads;
    end);
end;

procedure TURLCheckerThread.UpdateStatus(const Msg: TStringList);
begin
  if Assigned(FOnStatus) then
     TThread.Synchronize(TThread.Current, procedure
       begin
         FOnStatus(Msg);
       end);
end;

procedure TURLCheckerThread.Execute;
var
  FUrl    : string;
  FHttp   : TIdHTTP;
  FResult : TStringList;
begin
  FHttp   := TIdHTTP.Create(nil);
  FResult := TStringList.Create;
    try
      while not Terminated and not FShouldStop^ do
        begin
          FUrl := '';
          FCriticalSection.Acquire;
            try
              if FUrlList.Count > 0 then
                begin
                  FUrl := FUrlList[0];
                  FUrlList.Delete(0);
                end;
            finally
              FCriticalSection.Release;
            end;

          if FUrl = '' then Break;

          try
            Sleep(Random(2000));  {Hier kommt später meine Aufgabe}
//          Http.Head(Url);

            FCriticalSection.Acquire;   {Keine Ahnung ob es richtig ist, habe aber rausgelesen dass man StringList auf jeden Fall mit CriticalSection absichern soll}
              try
                FResult.Add('OK: ' + FUrl);
                FResult.Add('Zweite Zeile');
              finally
                FCriticalSection.Release;
              end;
          except
            on E: Exception do
              begin
                FResult.Add('FAIL: ' + FUrl + ' - ' + E.Message);
              end;
          end;

          UpdateStatus(FResult);

          TThread.Synchronize(nil, procedure
            begin
              Dec(Form1.IdleJobs);
              Form1.UpdateLabelJobs;

              Inc(Form1.JobsFinished);
              Form1.UpdateLabelFinished;
            end);
        end;
    finally
      FreeAndNil(FHttp);
      FreeAndNil(FResult);

      TThread.Synchronize(nil, procedure
        begin
          Dec(Form1.ActiveThreads);
          Form1.UpdateLabelActiveThreads;
        end);
    end;
end;

procedure TForm1.FormDestroy(Sender: TObject);
begin
  FreeAndNil(CriticalSection);
  FreeAndNil(UrlList);
end;

procedure TForm1.ButtonStartClick(Sender: TObject);
var
  Threads : array[0..99] of TURLCheckerThread;
  Index   : Integer;
begin
  if Assigned(CriticalSection) then FreeAndNil(CriticalSection); {Falls ich nach Cancel noch mal Start drücke statt Weiter}
  if Assigned(UrlList)         then FreeAndNil(UrlList);         {Falls ich nach Cancel noch mal Start drücke statt Weiter}

  UrlList         := TStringList.Create;
  CriticalSection := TCriticalSection.Create;
  ShouldStop      := False;

  for Index := 1 to 100 do
    begin
      URLList.Add('http://google.com/' + IntToStr(Index));
    end;

  LabelJobs.Caption := '100';
  LabelIdle.Caption := '100';
  IdleJobs          := 100;
  JobsFinished      := 0;

  for Index := 1 to SpinEditThreads.Value do
    Threads[Index] := TURLCheckerThread.Create(UrlList, CriticalSection,
      procedure(Msg: TStringList)
        begin
          RichEditLog.Lines.AddStrings(Msg);  {Statusanzeige, z.B. in Memo oder Listbox  Msg = das Ergebnis vom Thread}
        end, @ShouldStop);
end;

procedure TForm1.ButtonWeiterClick(Sender: TObject);
var
  Threads : array[0..99] of TURLCheckerThread;
  Index   : Integer;
begin
  ShouldStop := False;

  for Index := 1 to SpinEditThreads.Value do
    Threads[Index] := TURLCheckerThread.Create(UrlList, CriticalSection,
      procedure(Msg: TStringList)
        begin
          RichEditLog.Lines.AddStrings(Msg);    {Statusanzeige, z.B. in Memo oder Listbox  Msg = das Ergebnis vom Thread}
        end, @ShouldStop);
end;

procedure TForm1.ButtonAbbruchClick(Sender: TObject);
begin
  ShouldStop := True;
end;

procedure TForm1.UpdateLabelActiveThreads;
begin
  TThread.Synchronize(nil, procedure
    begin
      LabelActiveThreads.Caption := IntToStr(ActiveThreads);
    end);
end;

procedure TForm1.UpdateLabelJobs;
begin
  TThread.Synchronize(nil, procedure
    begin
      LabelIdle.Caption     := IntToStr(IdleJobs);
    end);
end;

procedure TForm1.UpdateLabelFinished;
begin
  TThread.Synchronize(nil, procedure
    begin
      LabelFinished.Caption := IntToStr(JobsFinished);
    end);
end;

procedure TForm1.ButtonEXITClick(Sender: TObject);
begin
  ShouldStop := True;
  Close;
end;

Allgemeine Fehler im Code bitte auch aufzeigen, allerdings unbedingt mit Erklärung für Non-Profi.
Momwentan bin ich stark überladen mit dem Zeug.

[jaenicke]

Zunächst einmal zu ein paar Fehlern:

Delphi-Quellcode:

constructor TURLCheckerThread.Create(UrlList: TStrings; CriticalSection: TCriticalSection; OnStatus: TProc<string>; ShouldStop: PBoolean; ResultList: TStrings);
begin
  inherited Create(False);
  FUrlList := UrlList;
  FResultList := ResultList;
  FCriticalSection := CriticalSection;
  FOnStatus := OnStatus;
  FShouldStop := ShouldStop;
  FreeOnTerminate := True;

  {Zähle aktive Threads}
  TThread.Synchronize(nil, procedure
    begin
      Inc(Form1.ActiveThreads);
      Form1.UpdateLabelActiveThreads;
    end);
end;

procedure TURLCheckerThread.UpdateStatus(const Msg: string);
begin
  if Assigned(FOnStatus) then
     TThread.Synchronize(TThread.Current, procedure begin FOnStatus(Msg); end);
end;

Erstens verwendest du TThread.Synchronize in TURLCheckerThread.Create. TURLCheckerThread.Create rufst du aber im Hauptthread auf. Deshalb brauchst du hier keine Synchronisation. In aktuellen Delphiversionen wird das korrekt behandelt, weshalb es hier keinen Fehler gibt.

Zweitens rufst du in UpdateStatus die Synchronisation mit

Delphi-Quellcode:

TThread.Synchronize(TThread.Current...

auf. Damit kannst du dir die Synchronisation auch sparen, denn wenn du mit dem aktuellen Thread synchronisierst, bleibst du im gleichen Threrad. Wenn du nil verwendest, wird der Code im Hauptthread ausgeführt.

Drittens packst du die Threads nach der Erstellung in ein Array. Du verwendest aber FreeOnTerminate, sprich der Thread wird nach Beendigung seiner Aufgabe automatisch freigegeben. Du weißt aber nicht, wann das passiert. Deshalb darfst du einen solchen Thread nie in eine Variable speichern, damit du nicht aus Versehen darauf zugreifst. Du weißt ja dann gar nicht, ob der Thread noch existiert, so dass das Fehler geben kann.

Viertens ist zwar kein Fehler, aber sehr ungünstig und fast so schlecht wie ein Fehler:
Du greifst aus deiner Threadklasse mit Form1.UpdateLabelActiveThreads auf Form1 zu. Du solltest stattdessen die Threadklasse besser in eine eigene Unit packen und alles, was diese braucht, dorthin übergeben. Dann schaffst du dir keine unnötigen Abhängigkeiten und hast eine bessere Trennung des Ablaufs im Thread.

Nun zum Thema:
Bei Threads muss man sich immer anschauen, was wie viel Zeit benötigt. Wenn man z.B. Threads hat, die einzeln nicht so lange brauchen, muss man aufpassen, dass man für deren Verwaltung nicht zu viel Aufwand betreibt. Denn ansonsten wird die Aufgabe insgesamt mit Threads nicht schneller erledigt als ohne.

In deinem Fall brauchst du viel Zeit, um die URLs zu prüfen. Daher ist der Aufwand für die Listen im Vergleich viel geringer. Deshalb ist es gar kein Problem, wenn jeder Thread eine eigene Ergebnisliste bekommt, die du dann im OnStatus aus dem Thread übergeben bekommst. Und die URL kannst du ja einfach direkt als String übergeben. Denn in der Schleife bei der Erstellung der Threads läuft ja nichts parallel, so dass du einfach direkt auf die URL-Liste zugreifen kannst.

Sprich als Pseudocode:

Delphi-Quellcode:

  for Index := 0 to SpinEditThreads.Value - 1 do
  begin
    TURLCheckerThread.Create(UrlList[i], ...);
    Inc(Form1.ActiveThreads);
    Form1.UpdateLabelActiveThreads;
  end;

So erstellst du aber keine neuen Threads, wenn die ersten URLs abgearbeitet sind. Deshalb wären ein Threadpool oder auch TParallel.For Alternativen.

Du kannst aber auch einfach die Liste auf die Anzahl der Threads verteilen und jedem Thread nicht eine URL geben, sondern eine Liste, die dieser dann abarbeitet. Sprich wenn du 300 URLs hast und 5 Threads möchtest, gibst du jedem Thread eine Liste von 60 URLs.

Das Thema Multithreading ist tatsächlich sehr umfangreich. Das ist nur ein Anfang. Dazu ließe sich noch viel mehr schreiben, aber besser ist glaube ich, wenn du dir das erst einmal anschaust und ggf. Rückfragen stellst.

[IMPEGA]

OK, das sind viele Fehler. Ich kann nur nach und nach versuchen, es zu verbessern.
Einige Sachen waren mir recht klar, dass es nicht gut ist, dass es aber so viel ist, hätte ich nicht gedacht.
Ich werde versuchen etwas zu korrigieren und frage erneut. Allerdings Step by Step.

Nur, damit ich mir etwas Arbeit sparen kann, und nicht erst die falsche Methode zu reparieren versuche.
Welche Methode sollte ich mir vornehmen. Welche hat eben mehr Potenzial?
Ich denke, herausgelesen zu haben, die erste wäre sicherer. Meine Kenntnisse, wie man sehen kann, reichen eher nicht aus, um es selbst zu bestimmen.

[Olli73]

Zitat:

Zitat von jaenicke (Beitrag 1548906)

Zweitens rufst du in UpdateStatus die Synchronisation mit

Delphi-Quellcode:

TThread.Synchronize(TThread.Current...

auf. Damit kannst du dir die Synchronisation auch sparen, denn wenn du mit dem aktuellen Thread synchronisierst, bleibst du im gleichen Threrad. Wenn du nil verwendest, wird der Code im Hauptthread ausgeführt.

Ist es nicht so, dass Synchronize immer zum Ausführen im Hauptthread führt und der Paramer für den Thread einfach nur eine optionale Information ist?

Zitat:

Synchronize bewirkt, dass der von AMethod festgelegte Aufruf im Haupt-Thread ausgeführt wird. Dabei werden Multithread-Konflikte verhindert. Der Parameter AThread wird dem Thread des Aufrufers zugeordnet.

Für statische Methoden können Sie AMethod jedem Thread mithilfe des Parameters AThread zuordnen. Außerdem können Sie nil/NULL als Parameter AThread verwenden, wenn Sie die Informationen für den Thread des Aufrufers im Haupt-Thread nicht brauchen.

Jedenfalls habe ich mir gerade selbst etwas programiert, um in einen anderen Thread (als den Hauptthread) synchronisieren zu können.

[Olli73]

Ich denke auch, dass es passieren kann, dass das Update im Hauptthread ausgeführt werden kann und auf die Ergebnisliste zugreift, während gleichzeitig ein anderer Thread auf die Ergebnisliste zugreift (Beispiel 1).

[IMPEGA]

Erstmal vielen Dank für die Antworten.
Ich bleibe nun bei meinem ersten Beispiel, und gehe damit weiter.

OK, wie gesagt, ich versuche Step by Step mein Code etwas zu verbessern.
Erstmal die einfachen Sachen, die mir klar sind und ich verstanden habe, warum es anders muss.
Korrigierter Code kommt gleich.

Um es übersichtlich zu halten, stelle ich erst mal weitere Fragen.

Zitat:

Drittens packst du die Threads nach der Erstellung in ein Array. Du verwendest aber FreeOnTerminate, sprich der Thread wird nach Beendigung seiner Aufgabe automatisch freigegeben. Du weißt aber nicht, wann das passiert. Deshalb darfst du einen solchen Thread nie in eine Variable speichern, damit du nicht aus Versehen darauf zugreifst. Du weißt ja dann gar nicht, ob der Thread noch existiert, so dass das Fehler geben kann.

Damit komme ich erst gar nicht klar, wie ich es anders machen kann. Bitte um ein kleines Beispiel, wie du es meinst. Hier verstehe ich es nicht wirklich. Ich dachte, es betrifft immer nur ein Thread.
Ich erkenne es auch nicht, wieso du vom Array sprichst. Weil, sehe oben : Ich dachte, es betrifft immer nur ein Thread.
Bitte erneut um Aufklärung für Non-Pfofi. Mir ist klar, dass ich die StringList mit allen aktiven Threads füttern kann, (das will ich aber nicht) allerdings erkenne ich es nicht wie da ein Array gebildet wird und wie ich dementsprechend die Threads anders terminieren soll.

Zitat:

Viertens ist zwar kein Fehler, aber sehr ungünstig und fast so schlecht wie ein Fehler:
Du greifst aus deiner Threadklasse mit Form1.UpdateLabelActiveThreads auf Form1 zu. Du solltest stattdessen die Threadklasse besser in eine eigene Unit packen und alles, was diese braucht, dorthin übergeben. Dann schaffst du dir keine unnötigen Abhängigkeiten und hast eine bessere Trennung des Ablaufs im Thread.

Das scheint mir (zurzeit) noch meine Fähigkeiten zu überfordern. Ein Beispiel zum Verstehen wäre aber auch super, auch wenn ich fürs Erste nur das Grobe reparieren möchte.
Das Thema ist recht schwer, ich versuche aber so gut es geht es etwas zu verstehen, es ist aber echt nicht wirklich leicht.

Zitat:

Threadpool oder auch TParallel.For Alternativen.

Das habe ich schon versucht. Für meine Zwecke habe ich schon mit ThreadPool Erfolge gefeiert. In diesem Fall klappte es aber nicht. Ich habe mich für diese Methode entschieden, weil es wenigstens ansatzweise für mich in echten Fall umsetzbar ist.

Zitat:

Du kannst aber auch einfach die Liste auf die Anzahl der Threads verteilen und jedem Thread nicht eine URL geben, sondern eine Liste, die dieser dann abarbeitet. Sprich wenn du 300 URLs hast und 5 Threads möchtest, gibst du jedem Thread eine Liste von 60 URLs.

Ich möchte nicht ins Detail gehen, es ist aber nicht möglich, die Liste zu teilen.

Delphi-Quellcode:

    procedure ButtonAbbruchClick(Sender: TObject);
    procedure ButtonEXITClick(Sender: TObject);
    procedure ButtonStartClick(Sender: TObject);
    procedure FormDestroy(Sender: TObject);
    procedure ButtonWeiterClick(Sender: TObject);
  private
    { Private-Deklarationen }
    CriticalSection  : TCriticalSection;
    UrlList          : TStringList;
    ErgebnisListe    : TStringList;
    ActiveThreads    : Int64;
    IdleJobs         : Int64;
    JobsFinished     : Int64;
    procedure UpdateLabelActiveThreads;
    procedure UpdateLabelJobs;
    procedure UpdateLabelFinished;
  public
    { Public-Deklarationen }
  end;

var
  Form1: TForm1;
  ShouldStop: Boolean = False;

implementation

{$R *.dfm}

type
  TURLCheckerThread = class(TThread)
  private
    FUrlList         : TStrings;
    FResultList      : TStrings;
    FCriticalSection : TCriticalSection;
    FOnStatus        : TProc<string>;
    FShouldStop      : PBoolean;
  protected
    procedure Execute; override;
    procedure UpdateStatus(const Msg: string);
  public
    constructor Create(UrlList: TStrings; CriticalSection: TCriticalSection; OnStatus: TProc<string>; ShouldStop: PBoolean; ResultList: TStrings);
  end;

{ TWorkerThread }
constructor TURLCheckerThread.Create(UrlList: TStrings; CriticalSection: TCriticalSection; OnStatus: TProc<string>; ShouldStop: PBoolean; ResultList: TStrings);
begin
  inherited Create(False);
  FUrlList         := UrlList;
  FResultList      := ResultList;
  FCriticalSection := CriticalSection;
  FOnStatus        := OnStatus;
  FShouldStop      := ShouldStop;
  FreeOnTerminate  := True;
end;

procedure TURLCheckerThread.UpdateStatus(const Msg: string);
begin
  if Assigned(FOnStatus) then
     TThread.Synchronize(nil, procedure begin FOnStatus(Msg); end);
end;

procedure TURLCheckerThread.Execute;
var
  Url  : string;
  Http : TIdHTTP;
begin
  Http := TIdHTTP.Create(nil);
    try
      while not Terminated and not FShouldStop^ do
        begin
          Url := '';
          FCriticalSection.Acquire;
            try
              if FUrlList.Count > 0 then
                begin
                  Url := FUrlList[0];
                  FUrlList.Delete(0);
                end;
            finally
              FCriticalSection.Release;
            end;

          if Url = '' then Break;

          try
            Sleep(Random(2000)); {Hier kommt später meine Aufgabe}
//          Http.Head(Url);

            FCriticalSection.Acquire;              {Keine Ahnung ob es richtig ist, habe aber rausgelesen dass man StringList auf jeden Fall mit CriticalSection absichern soll}
              try
                FResultList.Add('OK: ' + Url);     {Ergebnis in StringList eintragen}
                FResultList.Add('Zweite Zeile');   {Ergebnis in StringList eintragen}
                FResultList.Add('Dritte Zeile');   {Ergebnis in StringList eintragen}
              finally
                FCriticalSection.Release;
              end;

//          UpdateStatus('OK: ' + Url);   {Damit kann ich zB. die Status ON oder OFF ausgeben, oder weglassen und NUR FResult als Ergebnis nutzen}
          except
            on E: Exception do UpdateStatus('FAIL: ' + Url + ' - ' + E.Message);
          end;

          TThread.Synchronize(nil, procedure
            begin
              Dec(Form1.IdleJobs);
              Form1.UpdateLabelJobs;

              Inc(Form1.JobsFinished);
              Form1.UpdateLabelFinished;
            end);
        end;
    finally
      Http.Free;

      TThread.Synchronize(nil, procedure
        begin
          Dec(Form1.ActiveThreads);
          Form1.UpdateLabelActiveThreads;
        end);
    end;
end;

procedure TForm1.FormDestroy(Sender: TObject);
begin
  FreeAndNil(CriticalSection);
  FreeAndNil(ErgebnisListe);
  FreeAndNil(UrlList);
end;

procedure TForm1.ButtonStartClick(Sender: TObject);
var
  Threads : array[0..99] of TURLCheckerThread;
  Index   : Integer;
begin
  if Assigned(CriticalSection) then FreeAndNil(CriticalSection); {Falls ich nach Cancel noch mal Start drücke statt Weiter}
  if Assigned(UrlList)         then FreeAndNil(UrlList);         {Falls ich nach Cancel noch mal Start drücke statt Weiter}

  UrlList         := TStringList.Create;
  CriticalSection := TCriticalSection.Create;
  ShouldStop      := False;

  for Index := 1 to 100 do
    begin
      URLList.Add('http://google.com/' + IntToStr(Index));
    end;

  LabelJobs.Caption := '100';
  LabelIdle.Caption := '100';
  IdleJobs          := 100;
  JobsFinished      := 0;

  ErgebnisListe := TStringList.Create;

  for Index := 1 to SpinEditThreads.Value do
    begin
      Threads[Index] := TURLCheckerThread.Create(UrlList, CriticalSection,
        procedure(Msg: string)
          begin
            RichEditLog.Lines.Add(Msg);                {Ergebnis als String}
            RichEditLog.Lines.Add(ErgebnisListe.Text); {Ergebnis als StringList}
            ErgebnisListe.Clear;                       {Nicht vergessen, sonst wird die StringList immer weiter befüllt}
          end, @ShouldStop, ErgebnisListe);

     {Zähle aktive Threads}
      Inc(ActiveThreads);
      LabelActiveThreads.Caption := IntToStr(ActiveThreads);
    end;
end;

procedure TForm1.ButtonWeiterClick(Sender: TObject);
var
  Threads : array[0..99] of TURLCheckerThread;
  Index   : Integer;
begin
  ShouldStop := False;

  for Index := 1 to SpinEditThreads.Value do
    begin
      Threads[Index] := TURLCheckerThread.Create(UrlList, CriticalSection,
        procedure(Msg: string)
          begin
            RichEditLog.Lines.Add(Msg);                {Ergebnis als String}
            RichEditLog.Lines.Add(ErgebnisListe.Text); {Ergebnis als StringList}
            ErgebnisListe.Clear;                       {Nicht vergessen, sonst wird die StringList immer weiter befüllt}
          end, @ShouldStop, ErgebnisListe);

     {Zähle aktive Threads}
      Inc(ActiveThreads);
      LabelActiveThreads.Caption := IntToStr(ActiveThreads);
    end;
end;

procedure TForm1.ButtonAbbruchClick(Sender: TObject);
begin
  ShouldStop := True;
end;

procedure TForm1.UpdateLabelActiveThreads;
begin
  LabelActiveThreads.Caption := IntToStr(ActiveThreads); {Das war eigentlich nur ein Versehen mit Synchronize. Es war schon spät und viel Stoff für einen Abend :)}
end;

procedure TForm1.UpdateLabelJobs;
begin
  LabelIdle.Caption := IntToStr(IdleJobs);
end;

procedure TForm1.UpdateLabelFinished;
begin
  LabelFinished.Caption := IntToStr(JobsFinished);
end;

procedure TForm1.ButtonEXITClick(Sender: TObject);
begin
  ShouldStop := True;
  Close;
end;

[Olli73]

Zitat:

Zitat von Olli73 (Beitrag 1548914)

Ich denke auch, dass es passieren kann, dass das Update im Hauptthread ausgeführt werden kann und auf die Ergebnisliste zugreift, während gleichzeitig ein anderer Thread auf die Ergebnisliste zugreift (Beispiel 1).

das hast du immer noch nicht beachtet.

[IMPEGA]

Zitat:

Zitat von Olli73 (Beitrag 1548919)

Zitat:

Zitat von Olli73 (Beitrag 1548914)

Ich denke auch, dass es passieren kann, dass das Update im Hauptthread ausgeführt werden kann und auf die Ergebnisliste zugreift, während gleichzeitig ein anderer Thread auf die Ergebnisliste zugreift (Beispiel 1).

das hast du immer noch nicht beachtet.

Und wie fern soll mir deine Antwort helfen?
Wenn du einen Fehler siehst, versuche doch mir zu helfen, statt nur zu sagen, dass es falsch ist.
Gib mir wenigstens einen Ansatz, wonach ich suchen soll, schreibe kleines Beispiel oder wie auch immer.

Nur zu sagen, was ich nicht beachtet habe, brauchst du nicht, es vermüllt nur den Thread. Bitte, sachlich oder gar nicht antworten. Es ist schwer genug für mich, das Ganze zu verstehen.
jaenicke hat schon einiges klar definiert, sachliche Vorschläge gemacht, so dass ich es wenigstens etwas nachvollziehen konnte.
Nun bin ich am Versuchen es zu korrigieren, allerdings was nicht im Kopf steck, springt dort nicht rein, nur weil du sagst, ich habe es nicht behandelt.
Ja, ich habe es nicht behandelt, weil ich noch keine Ahnung habe, wie ich es anfassen soll.

Mit deinen Antworten komme ich echt schlecht zu Recht, es demotiviert mich weiter zu fragen, also bitte lasse es sein. Danke.

[himitsu]

Wenn es um Objekte geht, braucht man nicht unbedingt eine sonstwo rumliegende CriticalSection.

Delphi bringt auch ein TMonitor mit (System.TMonitor, weil der Name wurde einfach nur extrem ungünstig raubkopiert).
Und damit kann man sowas, wie eine CriticalSection an jedes Delphi-Objekt hängen.

Delphi-Quellcode:

System.TMonitor.Enter(Obj);  // oder TryEnter
try
  ...
finally
  System.TMonitor.Exit(Obj);
end;

Mir war so, als gab es schon eine threadsicher Queue/Stack in Delphi, aber die bei Beiden in den Generics sind es leider nicht.
Irgendwo gibt es aber zumindest eine thread-save TList (leider nicht die generische).

Gerade eine Queue macht sich, zur Verwaltung/Abarbeitung von Jobs recht gut.

[Olli73]

Wenn du in einem Thread "UpdateStatus" aufrufst wird das hier im Hauptthread (durch das Synchronize) aufgerufen.

Delphi-Quellcode:

      Threads[Index] := TURLCheckerThread.Create(UrlList, CriticalSection,
        procedure(Msg: string)
          begin
            RichEditLog.Lines.Add(Msg); {Ergebnis als String}
            RichEditLog.Lines.Add(ErgebnisListe.Text); {Ergebnis als StringList}
            ErgebnisListe.Clear; {Nicht vergessen, sonst wird die StringList immer weiter befüllt}
          end, @ShouldStop, ErgebnisListe);

Dabei greifst du auf die (globale) Variable "Ergebnisliste" zu. Wenn ein weiterer Thread aber gerade dabei ist in diese Ergebnisliste zu schreiben, kommt es zu Problemen.

Lösung: Auch dort die CriticalSection benutzen.

Des weiteren nutzt du 1 CriticalSection für 2 unterschiedliche Listen. Das funktioniert zwar, aber du sperrst immer 2 Listen, was das ganze verlangsamen kann.

Lösung: 2 CriticalSections oder TMonior.Enter(Liste) verwenden.

[Olli73]

Zitat:

Zitat von himitsu (Beitrag 1548922)

Mir war so, als gab es schon eine threadsicher Queue/Stack in Delphi, aber die bei Beiden in den Generics sind es leider nicht.
Irgendwo gibt es aber zumindest eine thread-save TList (leider nicht die generische).

https://docwiki.embarcadero.com/Libr...ns.TThreadList

https://docwiki.embarcadero.com/Libr...TThreadedQueue

Das hier?

[IMPEGA]

Zitat:

Zitat von Olli73 (Beitrag 1548923)

Wenn du in einem Thread "UpdateStatus" aufrufst wird das hier im Hauptthread (durch das Synchronize) aufgerufen.

Delphi-Quellcode:

      Threads[Index] := TURLCheckerThread.Create(UrlList, CriticalSection,
        procedure(Msg: string)
          begin
            RichEditLog.Lines.Add(Msg); {Ergebnis als String}
            RichEditLog.Lines.Add(ErgebnisListe.Text); {Ergebnis als StringList}
            ErgebnisListe.Clear; {Nicht vergessen, sonst wird die StringList immer weiter befüllt}
          end, @ShouldStop, ErgebnisListe);

Dabei greifst du auf die (globale) Variable "Ergebnisliste" zu. Wenn ein weiterer Thread aber gerade dabei ist in diese Ergebnisliste zu schreiben, kommt es zu Problemen.

Lösung: Auch dort die CriticalSection benutzen.

Des weiteren nutzt du 1 CriticalSection für 2 unterschiedliche Listen. Das funktioniert zwar, aber du sperrst immer 2 Listen, was das ganze verlangsamen kann.

Lösung: 2 CriticalSections oder TMonior.Enter(Liste) verwenden.

Das mit CriticalSection gedoppelt, das vielleicht später, nun habe ich versucht die StringList zu individualisieren. Wie gesagt, ich gehe nur Step by Step.
Nur als grobe Info. Kann ich einfach eine Zweite CriticalSection einbauen und gut ist? Die eine bleibt für FUrlList und eine zweite für FResultList ???
@himitsu Sorry, auf deinen Vorschlag gehe ich erst nicht ein, bin eh schon am Limit. Für heute reicht es mir, glaube ich.

Hier noch meine Änderung, jetzt sollte je Thread eine Liste benutzt werden.

Delphi-Quellcode:

    procedure ButtonStartClick(Sender: TObject);
    procedure ButtonAbbruchClick(Sender: TObject);
    procedure ButtonWeiterClick(Sender: TObject);
    procedure ButtonEXITClick(Sender: TObject);
  private
    { Private-Deklarationen }
    CriticalSection  : TCriticalSection;
    UrlList          : TStringList;
    ErgebnisListe    : TStringList;
    ActiveThreads    : Int64;
    IdleJobs         : Int64;
    JobsFinished     : Int64;
    procedure UpdateLabelActiveThreads;
    procedure UpdateLabelJobs;
    procedure UpdateLabelFinished;
  public
    { Public-Deklarationen }
  end;

var
  Form1      : TForm1;
  ShouldStop : Boolean = False;

implementation

{$R *.dfm}

type
  TURLCheckerThread = class(TThread)
  private
    FUrlList         : TStrings;
    FCriticalSection : TCriticalSection;
    FOnStatus        : TProc<TstringList>;
    FShouldStop      : PBoolean;
  protected
    procedure Execute; override;
    procedure UpdateStatus(const Msg: TstringList);
  public
    constructor Create(UrlList: TStrings; CriticalSection: TCriticalSection; OnStatus: TProc<TstringList>; ShouldStop: PBoolean);
  end;

{ TWorkerThread }
constructor TURLCheckerThread.Create(UrlList: TStrings; CriticalSection: TCriticalSection; OnStatus: TProc<TstringList>; ShouldStop: PBoolean);
begin
  inherited Create(False);
  FUrlList         := UrlList;
  FCriticalSection := CriticalSection;
  FOnStatus        := OnStatus;
  FShouldStop      := ShouldStop;
  FreeOnTerminate  := True;
end;

procedure TURLCheckerThread.UpdateStatus(const Msg: TstringList);
begin
  if Assigned(FOnStatus) then
     TThread.Synchronize(TThread.Current, procedure begin FOnStatus(Msg); end);
end;

procedure TURLCheckerThread.Execute;
var
  FResultList : TstringList;
  Url         : string;
  Http        : TIdHTTP;
begin
  Http := TIdHTTP.Create(nil);
    try
      while not Terminated and not FShouldStop^ do
        begin
          Url := '';
          FCriticalSection.Acquire;
            try
              if FUrlList.Count > 0 then
                begin
                  Url := FUrlList[0];
                  FUrlList.Delete(0);
                end;
            finally
              FCriticalSection.Release;
            end;

          if Url = '' then Break;

          Sleep(Random(2000)); {Hier kommt später meine Aufgabe}

          FResultList := TStringList.Create;            //--> Hier geändert  Pro Job wird eine Liste erstellt und zerstört
          try
            try
  //          Http.Head(Url);

              FCriticalSection.Acquire;              {Keine Ahnung ob es richtig ist, habe aber rausgelesen dass man StringList auf jeden Fall mit CriticalSection absichern soll}
                try
                  FResultList.Add('OK: ' + Url);     {Ergebnis in StringList eintragen}
                  FResultList.Add('Zweite Zeile');   {Ergebnis in StringList eintragen}
                  FResultList.Add('Dritte Zeile');   {Ergebnis in StringList eintragen}
                finally
                  FCriticalSection.Release;
                end;

              UpdateStatus(FResultList);
            except
              on E: Exception do
                begin
                  FResultList.Add('FAIL: ' + Url + ' - ' + E.Message);
                  UpdateStatus(FResultList);
                end;
            end;
          finally
            FreeAndNil(FResultList);                            //--> Hier geändert  Pro Job wird eine Liste erstellt und zerstört
          end;

          TThread.Synchronize(nil, procedure
            begin
              Dec(Form1.IdleJobs);
              Form1.UpdateLabelJobs;

              Inc(Form1.JobsFinished);
              Form1.UpdateLabelFinished;
            end);
        end;
    finally
      FreeAndNil(FResultList);
      FreeAndNil(Http);

      TThread.Synchronize(nil, procedure
        begin
          Dec(Form1.ActiveThreads);
          Form1.UpdateLabelActiveThreads;
        end);
    end;
end;

procedure TForm1.ButtonStartClick(Sender: TObject);
var
  Threads : array[0..99] of TURLCheckerThread;
  Index   : Integer;
begin
  if Assigned(CriticalSection) then FreeAndNil(CriticalSection); {Falls ich nach Cancel noch mal Start drücke statt Weiter}
  if Assigned(UrlList)         then FreeAndNil(UrlList);         {Falls ich nach Cancel noch mal Start drücke statt Weiter}

  UrlList         := TStringList.Create;
  CriticalSection := TCriticalSection.Create;
  ShouldStop      := False;

  for Index := 1 to 100 do
    begin
      URLList.Add('http://google.com/' + IntToStr(Index));
    end;

  LabelJobs.Caption := '100';
  LabelIdle.Caption := '100';
  IdleJobs          := 100;
  JobsFinished      := 0;


  for Index := 1 to SpinEditThreads.Value do
    begin
      Threads[Index] := TURLCheckerThread.Create(UrlList, CriticalSection,
        procedure(Msg: TstringList)
          var
            ErgebnisListe : TStringList;
          begin
            RichEditLog.Lines.Add(Msg.Text);                {Ergebnis als StringList}
          end, @ShouldStop);

     {Zähle aktive Threads}
      Inc(ActiveThreads);
      LabelActiveThreads.Caption := IntToStr(ActiveThreads);
    end;
end;

procedure TForm1.ButtonWeiterClick(Sender: TObject);
var
  Threads : array[0..99] of TURLCheckerThread;
  Index   : Integer;
begin
  ShouldStop := False;

  for Index := 1 to SpinEditThreads.Value do
    begin
      Threads[Index] := TURLCheckerThread.Create(UrlList, CriticalSection,
        procedure(Msg: TstringList)
          begin
            RichEditLog.Lines.Add(Msg.Text);              {Ergebnis als StringList}
          end, @ShouldStop);

     {Zähle aktive Threads}
      Inc(ActiveThreads);
      LabelActiveThreads.Caption := IntToStr(ActiveThreads);
    end;
end;

procedure TForm1.ButtonAbbruchClick(Sender: TObject);
begin
  ShouldStop := True;
end;

procedure TForm1.ButtonEXITClick(Sender: TObject);
begin
  ShouldStop := True;
  Close;
end;


procedure TForm1.UpdateLabelActiveThreads;
begin
  LabelActiveThreads.Caption := IntToStr(ActiveThreads);
end;

procedure TForm1.UpdateLabelJobs;
begin
  LabelIdle.Caption     := IntToStr(IdleJobs);
end;

procedure TForm1.UpdateLabelFinished;
begin
  LabelFinished.Caption := IntToStr(JobsFinished);
end;

EDIT, erst jetzt gesehen, da stimmt noch etwas nicht. Es reicht für heute. Sonst kommt nur noch Misst dabei raus.


Das Teil mag ich auch nicht, erstmal habe ich auch kein Plan wie ich es korrigieren/verbessern kann.

Delphi-Quellcode:

      TThread.Synchronize(nil, procedure
        begin
          Dec(Form1.ActiveThreads);
          Form1.UpdateLabelActiveThreads;
        end);

[jaenicke]

Ich habe einmal ein kleines Beispiel erstellt, siehe Anhang.

[IMPEGA]

Zitat:

Zitat von jaenicke (Beitrag 1548932)

Ich habe einmal ein kleines Beispiel erstellt, siehe Anhang.

Danke sehr,
das muss ich mir in Ruhe anschauen. Für heute habe ich genug davon.

[jaenicke]

Vielleicht noch zur Erklärung:
- Eine Modifikation einer Stringliste ist aufwendiger als eine Liste mit Zahlen, weshalb diese nur gelesen wird und stattdessen die Indizes in einer Liste stehen und abgearbeitet werden.
- Man könnte die Liste auch numerisch aufteilen, sprich die ersten 50 für den Thread, die nächsten für den nächsten, aber das hat einen wichtigen Nachteil:
Wenn manche URLs direkt antworten und andere erst nach einer gewissen Zeit, wäre dann ggf. ein Thread viel länger beschäftigt als alle anderen. Daher wird das einzeln verteilt, so dass es insgesamt am schnellsten geht.
- Die Anzahl der Threads mit 2 ist natürlich nur in der Demo so, damit man den Threadzähler sieht usw., der Wert sollte natürlich im echten Programm höher liegen.

[IMPEGA]

Erstmal,
vielen Dank für deine Vorlage.
Leider, bin ich mit der Vorlage noch mehr überfordert. Wie gesagt, Coden ist nur ein Hobby von mir.
Ich wage mich zwar an neue Themen ran, jedoch ist mein Wissen nicht gerade professionell.
Dein Muster müsste ich wieder komplett abarbeiten, viel dazu lernen.
Bei meinem Code verstehe ich das Meiste. Ich weiß zwar nicht, wie man es wirklich korrekt umsetzen soll, verstehen den Code tue ich aber schon.
Das meiste davon habe ich mir nämlich selbst ausgedacht. (Natürlich, mit der freundlichen Unterstützung von Google, und viel Lesen)
Ich bin sehr dankbar für die Vorlage, muss aber auf dem Boden bleiben und in meinen Möglichkeiten eine Lösung suchen.
Wenn es erstmal funktioniert, kann ich versuchen mich weiter zu orientieren.
Dich einfach nur kopieren, ohne Sinn und Verstand, mag ich nicht, möchte es auch nicht.

Also, stelle ich erstmal weitere Fragen zu meinem Code.
1: Darf ich einfach zweite CriticalSection deklarieren ??
2: Wie kann ich das hier besser umsetzen (Idee mit Erklärung, oder kleiner Muster wäre super) Dec(Form1.ActiveThreads); Form1.UpdateLabelActiveThreads;
3: Die StringList erstelle ich nun im Thread, eine pro Job und lösche sie, wen fertig. Gibt es damit Ärger? oder darf ich es so machen?

Delphi-Quellcode:

procedure TURLCheckerThread.Execute;
var
  FErgebnis : TstringList;               // --> Frage Nr. 3
  Url       : string;
  Http      : TIdHTTP;
begin
  Http := TIdHTTP.Create(nil);
    try
      while not Terminated and not FShouldStop^ do
        begin
          Url := '';
          FCS1.Acquire;                         // --> Frage Nr. 1
            try
              if FUrlList.Count > 0 then
                begin
                  Url := FUrlList[0];
                  FUrlList.Delete(0);
                end;
            finally
              FCS1.Release;                   // --> Frage Nr. 1
            end;

          if Url = '' then Break;

          Sleep(Random(2000)); {Hier kommt später meine Aufgabe}

          FErgebnis := TStringList.Create;                                       // --> Frage Nr. 3
            try
              try
    //          Http.Head(Url);

                FCS2.Acquire;                        // --> Frage Nr. 1
                  try
                    FErgebnis.Add('OK: ' + Url);    
                    FErgebnis.Add('Zweite Zeile');   
                    FErgebnis.Add('Dritte Zeile');   
                  finally
                    FCS2.Release;                   // --> Frage Nr. 1
                  end;

                UpdateStatus(FErgebnis);                                         // --> Frage Nr. 3
              except
                on E: Exception do
                  begin
                    FErgebnis.Add('FAIL: ' + Url + ' - ' + E.Message);
                    UpdateStatus(FErgebnis);
                  end;
              end;
            finally
              FreeAndNil(FErgebnis);                                             // --> Frage Nr. 3  
            end;

          TThread.Synchronize(nil, procedure
            begin
              Dec(Form1.IdleJobs);     // --> Frage Nr. 2
              Form1.UpdateLabelJobs;   // --> Frage Nr. 2

              Inc(Form1.JobsFinished);    // --> Frage Nr. 2
              Form1.UpdateLabelFinished;  // --> Frage Nr. 2 
            end);
        end;
    finally
      FreeAndNil(FErgebnis);
      FreeAndNil(Http);

      TThread.Synchronize(nil, procedure
        begin
          Dec(Form1.ActiveThreads);        // --> Frage Nr. 2
          Form1.UpdateLabelActiveThreads;  // --> Frage Nr. 2
        end);
    end;
end;

Falls ich damit kaum zum Erfolg kommen sollte, bitte auch aufklären warum, dann muss ich neuen Ansatz suchen.
Damit komme ich aber einigermaßen klar, also wäre es schon mein Favorit.
Auch wenns nicht perfekt ist. :)

.

[IMPEGA]

Ich habe ein wenig nachgebessert. Kann jemand noch mal den Code anschauen?
Besser kriege ich es wohl nicht hin.

Delphi-Quellcode:

    procedure FormDestroy(Sender: TObject);

    procedure ButtonStartClick(Sender: TObject);
    procedure ButtonAbbruchClick(Sender: TObject);
    procedure ButtonWeiterClick(Sender: TObject);
    procedure ButtonEXITClick(Sender: TObject);
  private
    { Private-Deklarationen }
    CSection1        : TCriticalSection;
    CSection2        : TCriticalSection;
    UrlList          : TStringList;
    ErgebnisListe    : TStringList;
    ActiveThreads    : Int64;
    IdleJobs         : Int64;
    JobsFinished     : Int64;
    procedure UpdateLabelActiveThreads(Sender: TObject);
    procedure UpdateLabelJobs(Sender: TObject);
    procedure UpdateLabelFinished(Sender: TObject);

  public
    { Public-Deklarationen }
  end;

var
  Form1      : TForm1;
  ShouldStop : Boolean = False;

implementation

{$R *.dfm}

type
  TURLCheckerThread = class(TThread)
  private
    FUpdateAktiv  : TNotifyEvent;   {<--- HIER}
    FUpdateJobs   : TNotifyEvent;   {<--- HIER}
    FUpdateFertig : TNotifyEvent;   {<--- HIER}
    FUrlList      : TStrings;
    FCSection1    : TCriticalSection;
    FCSection2    : TCriticalSection;
    FOnStatus     : TProc<TstringList>;
    FShouldStop   : PBoolean;
    FTimeOut      : Word;           {<--- HIER}
  protected
    procedure Execute; override;
    procedure UpdateStatus(const Msg: TstringList);
  public
    constructor Create(UrlList: TStrings; CSection1, CSection2: TCriticalSection; OnStatus: TProc<TstringList>; ShouldStop: PBoolean);

    property OnUpdateActive : TNotifyEvent read FUpdateAktiv  write FUpdateAktiv;   {<--- HIER}
    property OnUpdateJobs   : TNotifyEvent read FUpdateJobs   write FUpdateJobs;    {<--- HIER}
    property OnUpdateFertig : TNotifyEvent read FUpdateFertig write FUpdateFertig;  {<--- HIER}

    property TimeOut : Word read FTimeOut write FTimeOut;                           {<--- HIER}
  end;

{ TWorkerThread }
constructor TURLCheckerThread.Create(UrlList: TStrings; CSection1, CSection2: TCriticalSection; OnStatus: TProc<TstringList>; ShouldStop: PBoolean);
begin
  inherited Create(False);
  FUrlList        := UrlList;
  FCSection1      := CSection1;
  FCSection2      := CSection2;
  FOnStatus       := OnStatus;
  FShouldStop     := ShouldStop;
  FreeOnTerminate := True;
end;

procedure TURLCheckerThread.UpdateStatus(const Msg: TstringList);
begin
  if Assigned(FOnStatus) then
     TThread.Synchronize(TThread.Current, procedure begin FOnStatus(Msg); end);
end;

procedure TURLCheckerThread.Execute;
var
  FErgebnis : TstringList;
  Url       : string;
  Http      : TIdHTTP;
begin
  Http                := TIdHTTP.Create(nil);
  Http.ConnectTimeout := FTimeOut;  // 8000
  Http.ReadTimeout    := FTimeOut;
    try
      while not Terminated and not FShouldStop^ do
        begin
          Url := '';
          FCSection1.Acquire;
            try
              if FUrlList.Count > 0 then
                begin
                  Url := FUrlList[0];
                  FUrlList.Delete(0);
                end;
            finally
              FCSection1.Release;
            end;

          if Url = '' then Break;

          Sleep(Random(2000)); {Hier kommt später meine Aufgabe}

          FErgebnis := TStringList.Create;
            try
              try
    //          Http.Head(Url);

                FCSection2.Acquire;                        {Keine Ahnung ob es richtig ist, habe aber rausgelesen dass man StringList auf jeden Fall mit CriticalSection absichern soll}
                  try
                    FErgebnis.Add('OK: ' + Url);     {Ergebnis in StringList eintragen}
                    FErgebnis.Add('Zweite Zeile');
                    FErgebnis.Add('Dritte Zeile');
                  finally
                    FCSection2.Release;
                  end;

                UpdateStatus(FErgebnis);
              except
                on E: Exception do
                  begin
                    FErgebnis.Add('FAIL: ' + Url + ' - ' + E.Message);
                    UpdateStatus(FErgebnis);
                  end;
              end;
            finally
              FreeAndNil(FErgebnis);
            end;

          if Assigned(FUpdateJobs)   then
            begin
              TThread.Synchronize(nil, procedure
                begin
                   FUpdateJobs(Self);
                end);
            end;

          if Assigned(FUpdateFertig)   then
            begin
              TThread.Synchronize(nil, procedure
                begin
                   FUpdateFertig(Self);
                end);
            end;
        end;
    finally
      FreeAndNil(FErgebnis);
      FreeAndNil(Http);

      if Assigned(FUpdateAktiv)   then
        begin
          TThread.Synchronize(nil, procedure
            begin
               FUpdateAktiv(Self);
            end);
        end;
    end;
end;

procedure TForm1.FormDestroy(Sender: TObject);
begin
  FreeAndNil(ErgebnisListe);
  FreeAndNil(UrlList);
  FreeAndNil(CSection1);
  FreeAndNil(CSection2);
end;

procedure TForm1.ButtonStartClick(Sender: TObject);
var
  Threads : array[0..100] of TURLCheckerThread;
  Index   : Integer;
begin
  if Assigned(CSection1) then FreeAndNil(CSection1);  {Falls ich nach Cancel noch mal Start drücke statt Weiter}
  if Assigned(CSection2) then FreeAndNil(CSection2);
  if Assigned(UrlList)   then FreeAndNil(UrlList);

  UrlList    := TStringList.Create;
  CSection1  := TCriticalSection.Create;
  CSection2  := TCriticalSection.Create;
  ShouldStop := False;

  for Index := 0 to 200 do
    begin
      URLList.Add('http://google.com/' + IntToStr(Index));
    end;

  LabelJobs.Caption := '100';
  LabelIdle.Caption := '100';
  IdleJobs          := 100;
  Gauge1.MaxValue   := 100;
  Gauge1.Progress   := 0;
  JobsFinished      := 0;

  for Index := 1 to SpinEditThreads.Value do
    begin
      Threads[Index] := TURLCheckerThread.Create(UrlList, CSection1, CSection2,
        procedure(Msg: TstringList)
          var
            ErgebnisListe : TStringList;
          begin
            RichEditLog.Lines.Add(Msg.Text);                {Ergebnis als StringList}
          end, @ShouldStop);

      Threads[Index].TimeOut        := SpinEditTimeout.Value;
      Threads[Index].OnUpdateActive := UpdateLabelActiveThreads;
      Threads[Index].OnUpdateJobs   := UpdateLabelJobs;
      Threads[Index].OnUpdateFertig := UpdateLabelFinished;

     {Zähle aktive Threads}
      Inc(ActiveThreads);
      LabelActiveThreads.Caption := IntToStr(ActiveThreads);
    end;
end;

procedure TForm1.ButtonWeiterClick(Sender: TObject);
var
  Threads : array[0..100] of TURLCheckerThread;
  Index   : Integer;
begin
  ShouldStop := False;

  for Index := 1 to SpinEditThreads.Value do
    begin
      Threads[Index] := TURLCheckerThread.Create(UrlList, CSection1, CSection2,
        procedure(Msg: TstringList)
          begin
            RichEditLog.Lines.Add(Msg.Text);              {Ergebnis als StringList}
          end, @ShouldStop);

      Threads[Index].TimeOut        := SpinEditTimeout.Value;
      Threads[Index].OnUpdateActive := UpdateLabelActiveThreads;
      Threads[Index].OnUpdateJobs   := UpdateLabelJobs;
      Threads[Index].OnUpdateFertig := UpdateLabelFinished;

     {Zähle aktive Threads}
      Inc(ActiveThreads);
      LabelActiveThreads.Caption := IntToStr(ActiveThreads);
    end;
end;

procedure TForm1.ButtonAbbruchClick(Sender: TObject);
begin
  ShouldStop := True;
end;

procedure TForm1.ButtonEXITClick(Sender: TObject);
begin
  ShouldStop := True;
  Close;
end;

procedure TForm1.UpdateLabelActiveThreads;
begin
  Dec(ActiveThreads);
  LabelActiveThreads.Caption := IntToStr(ActiveThreads);
end;

procedure TForm1.UpdateLabelJobs(Sender: TObject);
begin
  Dec(Form1.IdleJobs);
  LabelIdle.Caption := IntToStr(IdleJobs);
end;

procedure TForm1.UpdateLabelFinished;
begin
  Inc(Form1.JobsFinished);
  LabelFinished.Caption := IntToStr(JobsFinished);
  Gauge1.Progress       := Gauge1.Progress + 1;
end;

[jaenicke]

Du packst die Threads nach wie vor in Threads[Index] und greifst darauf nach dem Start der Threads (du erstellt diese ja nicht suspended, das ist der Parameter an Create) noch zu.

Erstens kann das wie gesagt knallen und zweitens wird das Timeout in Execute ggf. schon gelesen, bevor du es von außen setzt.

Du musst alle Werte im Konstruktor übergeben oder suspended starten.

[IMPEGA]

Ok, also ist das Teil praktisch ...hmmm komplett falsch ?
Mit Timeout, nach deiner Erklärung kann ich nachvollziehen, daran habe ich leider nicht gedacht. Timeout kann ich irgendwie global setzen, oder eben in Create packen.
Das zu ändern, ist kein Problem.
Der Rest aber, findet doch erst statt wenn der Thread es meldet, also dürfte es nicht knallen. Sprich, die ganzen Rückmeldungen vom Thread kann ich so realisieren?
Ist das korrekt, oder liege ich wieder falsch? Ich wollte unbedingt den Zugriff vom Thread aus auf die Form1 vermeiden. Wie es am Anfang bemängelt wurde.

Delphi-Quellcode:

      Threads[Index].TimeOut := SpinEditTimeout.Value;               // Das ist Müll. Wird korrigiert

      Threads[Index].OnUpdateActive := UpdateLabelActiveThreads;
      Threads[Index].OnUpdateJobs := UpdateLabelJobs;
      Threads[Index].OnUpdateFertig := UpdateLabelFinished;

Sorry fürs ständige Nachfragen, so lerne ich aber (für meine Verhältnisse) recht viel dazu.
Danke noch mal fürs Unterstützen.


EDIT:
Ich habe es doch so umgebaut

Delphi-Quellcode:

constructor TURLCheckerThread.Create(UrlList: TStrings; CSection1, CSection2: TCriticalSection; OnStatus: TProc<TstringList>; ShouldStop: PBoolean; OnUpdateActive, OnUpdateJobs, OnUpdateFertig: TNotifyEvent; Timeout: Word);
begin
  inherited Create(False);
  FUrlList        := UrlList;
  FCSection1      := CSection1;
  FCSection2      := CSection2;
  FOnStatus       := OnStatus;
  FShouldStop     := ShouldStop;
  FreeOnTerminate := True;
  FTimeOut        := Timeout;
  FUpdateAktiv    := OnUpdateActive;
  FUpdateJobs     := OnUpdateJobs;
  FUpdateFertig   := OnUpdateFertig;
end;

und beim Start

Delphi-Quellcode:

procedure TForm1.ButtonStartClick(Sender: TObject);
var
  Threads : array[0..100] of TURLCheckerThread;
  Index   : Integer;
  Timeout : Word;
begin
  if Assigned(CSection1) then FreeAndNil(CSection1);  {Falls ich nach Cancel noch mal Start drücke statt Weiter}
  if Assigned(CSection2) then FreeAndNil(CSection2);
  if Assigned(UrlList)   then FreeAndNil(UrlList);

  UrlList    := TStringList.Create;
  CSection1  := TCriticalSection.Create;
  CSection2  := TCriticalSection.Create;
  ShouldStop := False;

  for Index := 0 to 200 do
    begin
      URLList.Add('http://google.com/' + IntToStr(Index));
    end;

  LabelActiveThreads.Caption := '0';
  LabelJobs.Caption          := '201';
  LabelIdle.Caption          := '201';
  IdleJobs                   := 201;
  Gauge1.MaxValue            := 201;
  Gauge1.Progress            := 0;
  JobsFinished               := 0;
  Timeout                    := SpinEditTimeout.Value * 1000;

  for Index := 1 to SpinEditThreads.Value do
    begin
      Threads[Index] := TURLCheckerThread.Create(UrlList, CSection1, CSection2,
        procedure(Msg: TstringList)
          var
            ErgebnisListe : TStringList;
          begin
            RichEditLog.Lines.Add(Msg.Text);                {Ergebnis als StringList}
          end, @ShouldStop, UpdateLabelActiveThreads, UpdateLabelJobs, UpdateLabelFinished, Timeout);

     {Zähle aktive Threads}
      Inc(ActiveThreads);
      LabelActiveThreads.Caption := IntToStr(ActiveThreads);
    end;
end;

[jaenicke]

Lass die Variable

Delphi-Quellcode:

Threads : array[0..100] of TURLCheckerThread;

einfach ganz weg. ;-)
Du darfst sie in deiner Konstellation doch ohnehin nach der Zuweisung nicht nutzen, also brauchst du sie nicht.

[IMPEGA]

Ok, das habe ich irgendwo zum Thema ThreadPool gelesen, erschien mir wichtig zu sein.
Das habe ich so weit korrigiert. Array ist weg.
Kann man mit dem Rest irgendwie leben?
Es muss nicht die optimale Lösung sein, doch etwas Craschsicher sollte es schon sein.
Für mich ist eben auch wichtig, dass ich den Code selbst verstehe und nachvollziehen kann, wo, was passiert.

Hier noch mal die Übersicht. Zum Thema Monitor habe ich noch nichts gelesen, ich gehe davon aus, dass mein CS1 und CS2 es korrekt abfangen.

Delphi-Quellcode:

    procedure FormCreate(Sender: TObject);
    procedure FormShow(Sender: TObject);
    procedure FormCanResize(Sender: TObject; var NewWidth, NewHeight: Integer; var Resize: Boolean);
    procedure FormDestroy(Sender: TObject);

    procedure ButtonStartClick(Sender: TObject);
    procedure ButtonAbbruchClick(Sender: TObject);
    procedure ButtonWeiterClick(Sender: TObject);
    procedure ButtonEXITClick(Sender: TObject);
  private
    { Private-Deklarationen }
    CSection1     : TCriticalSection;
    CSection2     : TCriticalSection;
    UrlList       : TStringList;
    ErgebnisListe : TStringList;
    ActiveThreads : Int64;
    IdleJobs      : Int64;
    JobsFinished  : Int64;
    procedure UpdateLabelActiveThreads(Sender: TObject);
    procedure UpdateLabelJobs(Sender: TObject);
    procedure UpdateLabelFinished(Sender: TObject);

  public
    { Public-Deklarationen }
  end;

var
  Form1      : TForm1;
  ShouldStop : Boolean = False;

implementation

{$R *.dfm}

type
  TURLCheckerThread = class(TThread)
  private
    FUpdateAktiv  : TNotifyEvent;
    FUpdateJobs   : TNotifyEvent;
    FUpdateFertig : TNotifyEvent;
    FUrlList      : TStrings;
    FCSection1    : TCriticalSection;
    FCSection2    : TCriticalSection;
    FOnStatus     : TProc<TstringList>;
    FShouldStop   : PBoolean;
    FTimeOut      : Word;
  protected
    procedure Execute; override;
    procedure UpdateStatus(const Msg: TstringList);
  public
    constructor Create(UrlList: TStrings; CSection1, CSection2: TCriticalSection; OnStatus: TProc<TstringList>; ShouldStop: PBoolean; OnUpdateActive, OnUpdateJobs, OnUpdateFertig: TNotifyEvent; Timeout: Word);
  end;

{ TWorkerThread }
constructor TURLCheckerThread.Create(UrlList: TStrings; CSection1, CSection2: TCriticalSection; OnStatus: TProc<TstringList>; ShouldStop: PBoolean; OnUpdateActive, OnUpdateJobs, OnUpdateFertig: TNotifyEvent; Timeout: Word);
begin
  inherited Create(False);
  FUrlList        := UrlList;
  FCSection1      := CSection1;
  FCSection2      := CSection2;
  FOnStatus       := OnStatus;
  FShouldStop     := ShouldStop;
  FreeOnTerminate := True;
  FTimeOut        := Timeout;
  FUpdateAktiv    := OnUpdateActive;
  FUpdateJobs     := OnUpdateJobs;
  FUpdateFertig   := OnUpdateFertig;
end;

procedure TURLCheckerThread.UpdateStatus(const Msg: TstringList);
begin
  if Assigned(FOnStatus) then
     TThread.Synchronize(TThread.Current, procedure begin FOnStatus(Msg); end);
end;

procedure TURLCheckerThread.Execute;
var
  FErgebnis : TstringList;
  Url       : string;
  Http      : TIdHTTP;
begin
  Http                := TIdHTTP.Create(nil);
  Http.ConnectTimeout := FTimeOut;
  Http.ReadTimeout    := FTimeOut;
    try
      while not Terminated and not FShouldStop^ do
        begin
          Url := '';

          FCSection1.Acquire;
            try
              if FUrlList.Count > 0 then
                begin
                  Url := FUrlList[0];
                  FUrlList.Delete(0);
                end;
            finally
              FCSection1.Release;
            end;

          if Url = '' then Break;

          Sleep(Random(2000)); {Hier kommt später meine Aufgabe}

          FErgebnis := TStringList.Create;
            try
              try
    //          Http.Head(Url);

                FCSection2.Acquire;                  {Ich habe aber gelesen dass man StringList auf jeden Fall mit CriticalSection absichern soll}
                  try
                    FErgebnis.Add('OK: ' + Url);     {Ergebnis in StringList eintragen}
                    FErgebnis.Add('Zweite Zeile');
                    FErgebnis.Add('Dritte Zeile');
                  finally
                    FCSection2.Release;
                  end;

                UpdateStatus(FErgebnis);
              except
                on E: Exception do
                  begin
                    FErgebnis.Add('FAIL: ' + Url + ' - ' + E.Message);
                    UpdateStatus(FErgebnis);
                  end;
              end;
            finally
              FreeAndNil(FErgebnis);
            end;

          if Assigned(FUpdateJobs)   then
            begin
              TThread.Synchronize(nil, procedure
                begin
                  FUpdateJobs(Self);
                end);
            end;

          if Assigned(FUpdateFertig)   then
            begin
              TThread.Synchronize(nil, procedure
                begin
                  FUpdateFertig(Self);
                end);
            end;
        end;
    finally
      FreeAndNil(FErgebnis);
      FreeAndNil(Http);

      if Assigned(FUpdateAktiv)   then
        begin
          TThread.Synchronize(nil, procedure
            begin
              FUpdateAktiv(Self);
            end);
        end;
    end;
end;

procedure TForm1.FormCreate(Sender: TObject);
var
  Icon : TIcon;
begin
  Icon        := TIcon.Create;
  Icon.Handle := LoadIcon(hInstance, 'ICON_1');
  Form1.Icon  := Icon;
  Icon.Free;
end;

procedure TForm1.FormShow(Sender: TObject);
var
  R : TRect;
begin
  Statusbar1.Perform($040A, 1, Integer(@R));
  Gauge1.Parent := Statusbar1;
  Gauge1.Top    := r.Top  + 2;
  Gauge1.Left   := r.Left + 2;
  Gauge1.Width  := r.Right  - r.Left - 4;
  Gauge1.Height := r.Bottom - r.Top  - 4;
end;

procedure TForm1.FormCanResize(Sender: TObject; var NewWidth, NewHeight: Integer; var Resize: Boolean);
begin
  FormShow(Sender); {Wiederverwenden der Logik, weil es genauso wie in TForm1.FormShow ist}
end;

procedure TForm1.FormDestroy(Sender: TObject);
begin
  FreeAndNil(ErgebnisListe);
  FreeAndNil(UrlList);
  FreeAndNil(CSection1);
  FreeAndNil(CSection2);
end;

procedure TForm1.ButtonStartClick(Sender: TObject);
var
  Threads : TURLCheckerThread;
  Index   : Integer;
  Timeout : Word;
begin
  if Assigned(CSection1) then FreeAndNil(CSection1);  {Falls ich nach Cancel noch mal Start drücke statt Weiter}
  if Assigned(CSection2) then FreeAndNil(CSection2);
  if Assigned(UrlList)   then FreeAndNil(UrlList);

  UrlList    := TStringList.Create;
  CSection1  := TCriticalSection.Create;
  CSection2  := TCriticalSection.Create;
  ShouldStop := False;

  for Index := 0 to 200 do
    begin
      URLList.Add('http://google.com/' + IntToStr(Index));
    end;

  LabelActiveThreads.Caption := '0';
  LabelJobs.Caption          := '201';
  LabelIdle.Caption          := '201';
  IdleJobs                   := 201;
  Gauge1.MaxValue            := 201;
  Gauge1.Progress            := 0;
  JobsFinished               := 0;
  Timeout                    := SpinEditTimeout.Value * 1000;

  for Index := 1 to SpinEditThreads.Value do
    begin
      Threads := TURLCheckerThread.Create(UrlList, CSection1, CSection2,
        procedure(Msg: TstringList)
          var
            ErgebnisListe : TStringList;
          begin
            RichEditLog.Lines.Add(Msg.Text); {Ergebnis vom Thread als StringList}
          end, @ShouldStop, UpdateLabelActiveThreads, UpdateLabelJobs, UpdateLabelFinished, Timeout);

     {Zähle aktive Threads}
      Inc(ActiveThreads);
      LabelActiveThreads.Caption := IntToStr(ActiveThreads);
    end;
end;

procedure TForm1.ButtonWeiterClick(Sender: TObject);
var
  Threads : TURLCheckerThread;
  Index   : Integer;
  Timeout : Word;
begin
  ShouldStop := False;
  Timeout    := SpinEditTimeout.Value * 1000;

  for Index := 1 to SpinEditThreads.Value do
    begin
      Threads := TURLCheckerThread.Create(UrlList, CSection1, CSection2,
        procedure(Msg: TstringList)
          begin
            RichEditLog.Lines.Add(Msg.Text); {Ergebnis vom Thread als StringList}
          end, @ShouldStop, UpdateLabelActiveThreads, UpdateLabelJobs, UpdateLabelFinished, Timeout);

     {Zähle aktive Threads}
      Inc(ActiveThreads);
      LabelActiveThreads.Caption := IntToStr(ActiveThreads);
    end;
end;

procedure TForm1.ButtonAbbruchClick(Sender: TObject);
begin
  ShouldStop := True;
end;

procedure TForm1.ButtonEXITClick(Sender: TObject);
begin
  ShouldStop := True;
  Close;
end;

procedure TForm1.UpdateLabelActiveThreads;
begin
  Dec(ActiveThreads);
  LabelActiveThreads.Caption := IntToStr(ActiveThreads);
end;

procedure TForm1.UpdateLabelJobs(Sender: TObject);
begin
  Dec(Form1.IdleJobs);
  LabelIdle.Caption := IntToStr(IdleJobs);
end;

procedure TForm1.UpdateLabelFinished;
begin
  Inc(Form1.JobsFinished);
  LabelFinished.Caption := IntToStr(JobsFinished);
  Gauge1.Progress       := Gauge1.Progress + 1;
end;

[Rollo62]

Zitat:

Zitat von jaenicke (Beitrag 1548932)

Ich habe einmal ein kleines Beispiel erstellt, siehe Anhang.

Sehr schönes Beispiel :thumb:

Ich frage mich, weil es eine globale Funktion ist, welche von allen Seiten aus angefragt werden könnte,
ob es nicht threadsicherer würde, wenn man die Parameter klonen würde?

Also hier

Delphi-Quellcode:

class procedure TUrlChecker.Check( const AUrlList        : TStrings;
                                   const AStatusCallback : TStatusCallback; 
                                         ThreadCount     : Integer;
                                   const AOnFinished     : TFinishedCallback );
var
  LUrlList : TStringList;
begin
  LUrlList : TStringList.Create;  // Better clone, to ensure higher threadsafety??
  LUrlList.AddStrings( AUrlList );

  TTask.Run(
    procedure
    var
        WorkQueue : TThreadedQueue< NativeUInt >;
        Tasks     : TArray<ITask>;
        I         : Integer;
    begin
      //               Intern nur die Kopie LUrlList, statt AUrlList verwenden
      if not Assigned( LUrlList ) or not Assigned( AStatusCallback ) then
          Exit;
      ...

Ich habe bei der direkten Nutzung von Parametern wie AUrlList über zwei anonyme Funktionen hinweg immer so meine Bauchschmerzen, ob sich da drin zwischendurch nicht doch was verändert hat.

In der Praxis wird es wohl 100% ausreichen, weil man so eine Funktion ja nur an einer bestimmten Stelle nutzt, die man gut beobachten kann, normalerweise.
Aber falls nicht, dann "better safe than sorry", solche Fehler sind wohl schwer zu finden.

Oder gibt es gute Gründe dafür, dass ich mir hier zu viele Sorgen wegen der Parameter mache :gruebel:

[jaenicke]

Mit dem aktuellen Delphi 12 kannst du diesen impliziten Scope, der die Werte solcher Variablen enthält, direkt in der Variablenliste sehen. Das Feature ist echt gut. Das hilft denke ich beim Verständnis deutlich weiter.

Wenn du z.B. einen out-Parameter dort hättest, würdest du die Meldung bekommen, dass dieser Wert nicht erfasst werden kann. Denn da der Funktionsaufruf schon vorbei sein kann, kann man das nicht über den Scope abbilden.

Hier in diesem Fall hast du natürlich Recht:
Die Stringliste könnte nach dem Aufruf freigegeben oder verändert werden. Ich habe mich darauf verlassen, dass während der Laufzeit der Threads niemand das Memo ändert. Mit deiner Änderung könnte man das Problem umgehen.

[Blup]

Der Code enthält noch einige Probleme:

"ErgebnisListe"
Wird an zwei Stellen deklariert (gefährlich), aber nur eine Variable wird tatsächlich initialisiert und wieder freigegeben,
ansonsten aber nicht verwendet, also überflüssig.

"CSection2"
Da auf die Stringliste "FErgebnis" niemals von zwei verschiedenen Thread gleichzeitig zugegriffen wird, ist diese Sektion überflüssig.
So wie diese jetzt verwendet wird, kann die auch nicht wirken.
Sinnvoller wäre an dieser Stelle statt der Stringliste den fertigen Protokollstring zurückzugeben ( FOnStatus : TProc<string>; )

"FErgebnis"
Wird noch einmal freigegeben, obwohl das nicht notwendig ist.

"ShouldStop"
Die Variable gehört nicht global deklariert, sondern zu diesem Anwendungsfall "private".
Der Inhalt der Variable wird per Referenz bei Erzeugung der Threads übergeben und dort als Referenz gespeichert.
Der Inhalt wird gleichzeitig durch die einzelnen Threads gelesen, aber auch durch den Hauptthread geändert.
Alle Zugriffe auf den Inhalt der Variablen müssen deshalb abgesichert werden (z.B. mit "CSection1").

"ButtonStartClick"
Bei erneutem Aufruf werden alle Objekte freigegeben.
Es wird aber nicht geprüft, ob eventuell noch Threads existieren, die mit diesen Objekten arbeiten.
Das würde dann vermutlich zum Absturz führen.

"ButtonWeiterClick"
Doppelter Code mit "ButtonStartClick", in eigene Untermethode auslagern.

"FUpdateAktiv"
Das Ereignis gehört eher in OnTerminate, das spart auch das Synchronize.

"FUpdateJobs" und "FUpdateFertig"
Zu einem Ereignis zusammenfassen und dort beide Aufgaben erledigen.

Bei jedem Synchronize wird der Thread angehalten.
Der Anwendungsfall könnte auf TThread.Queue() umgestellt werden.

[IMPEGA]

Ui, das nenne ich ausführlich. Vielen Dank.

Kleinigkeiten sofort korrigiert
"ErgebnisListe" - Das habe ich übersehen, ist in meinem Fall ganz überflüssig. Ich habe es üneral entfernt


"CSection2" - Das habe ich schon irgendwie vermutet, leider habe ich kein Plan wie man die CS richtig setzen soll. Mangel an Verständnis.
Kannst du an meinen Kode es zeigen bitte?

"ShouldStop" - Erneut Mangel an Wissen. Nach deiner Erklärung aber nachvollziehbar.

"ButtonStartClick" - Hier muss bei Weiter die Möglichkeit bestehen zu wählen, ob man weiter machen will oder nicht. Code ist nicht gleich. Später kommen noch paar weiter Abfragen.
Das mag ich so belassen wie es ist, für später Korekturen.

"FUpdateAktiv"
Das Ereignis gehört eher in OnTerminate, das spart auch das Synchronize

Das kann ich erst nicht wirklich umsetzen, muss erst darüber schlafen, und etwas lesen. Bei dem Punkt bin ich verwirrt.

"FUpdateJobs" und "FUpdateFertig"
Zu einem Ereignis zusammenfassen und dort beide Aufgaben erledigen.

Das wird sofort erledigt. Einleuchtend.

Danke sehr.
Ein Muster mit meiner CSection1 wäre super. Ich habe zwar viel darüber gelesen, bei der Menge an Sachen, bleibt nicht immer das Richtige im Kopf.

EDIT
Ich habe versucht es so anzupassen

Delphi-Quellcode:

procedure TMultiThread.Execute;
var
  FErgebnis : TstringList;
  Url       : string;
  Http      : TIdHTTP;
begin
  Http := TIdHTTP.Create(nil);
    try
      Http.ConnectTimeout := FTimeOut;
      Http.ReadTimeout    := FTimeOut;

      while not Terminated and not FShouldStop^ do
        begin
          Url := '';

          FCSection1.Acquire;
            try
              if FUrlList.Count > 0 then
                begin
                  Url := FUrlList[0];
                  FUrlList.Delete(0);
                end;

              if Url = '' then Break;

              FErgebnis := TStringList.Create;
                try
                  try
//                  Http.Head(Url);

                    Sleep(Random(5000));  {Simulation einer Verzögerung}

                    FErgebnis.Add('OK: ' + Url);    {Ergebnis in StringList eintragen}
                    FErgebnis.Add('Zweite Zeile');
                    FErgebnis.Add('Dritte Zeile');

                    UpdateErgebnis(FErgebnis);
                  except
                    on E: Exception do
                      begin
                        FErgebnis.Add('FAIL: ' + Url + ' - ' + E.Message);
                        UpdateErgebnis(FErgebnis);
                      end;
                  end;
                finally
                  FreeAndNil(FErgebnis);
                end;
            finally
              FCSection1.Release;
            end;

          if Assigned(FUpdateJob)   then
            begin
              TThread.Synchronize(nil, procedure
                begin
                  FUpdateJob(Self);
                end);
            end;
        end;
    finally
      FreeAndNil(Http);

      if Assigned(FUpdateAktiv)   then
        begin
          TThread.Synchronize(nil, procedure
            begin
              FUpdateAktiv(Self);
            end);
        end;
    end;
end;

Nun verhält sich das Ganze als ob es nur ein Thread ist.
Egal wie hoch ich Sleep setze. Antworten kommen immer von nachfolgendem Thread
Also immer gleich
OK: http://google.com/0
Zweite Zeile
Dritte Zeile

OK: http://google.com/1
Zweite Zeile
Dritte Zeile

OK: http://google.com/2
Zweite Zeile
Dritte Zeile

OK: http://google.com/3
Zweite Zeile
Dritte Zeile

OK: http://google.com/4
Zweite Zeile
Dritte Zeile

Die Antwortzeit ist natürlich unterschiedlich, wegen Random
Wenn ich die CS weglasse, kommen die Antworten immer durchgemischt, so wie man es von Threads erwartet. Auch mit unterschiedlichen Zeiten, doch sobald ein Thread fertig ist kommt die Antwort.
Also mache ich noch etwas falsch. Oder?

Wenn ich die CS durch
System.TMonitor.Enter(Self);
...
...
System.TMonitor.Exit(Self);

dann funktioniert es wie es soll.

Geht es überhaupt so? TMonitor habe ich noch nie benutzt.

[Blup]

Wenn man den ganzen Ablauf innerhalb einer CriticalSection hat, läuft da auch nichts mehr parallel.
Die ist nur für den Zugriff auf gemeinsam genutzte Ressourcen gedacht (z.B. Variablen im Speicher).
Ich hab mal versucht das ganze Threadsicher umzusetzen:

Anhang 57607