Zunächst einmal zu ein paar Fehlern:

zusammenfalten · markieren

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 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:

markieren

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.

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.

Zitat von jaenicke:

Zweitens rufst du in UpdateStatus die Synchronisation mit 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.

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

Zitat von 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).

das hast du immer noch nicht beachtet.

Zitat von Olli73:

Zitat von 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).

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.

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.

markieren

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.

Zitat von himitsu:

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?

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

markieren

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.

Zitat von Olli73:

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

markieren

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.

zusammenfalten · markieren

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.

markieren

Delphi-Quellcode:

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