CopyFile lässt die Anwendung hängen, wie umgehen?

[BUG]

Zitat von mm1256:

Das Problem - aus meiner Sicht - ist die Synchronisation des ProgressBar im Formular.

Mit dem synchronize von TThread ist alles Komplizierte (Aufruf von Methoden im Mainthread über Windows Messages) im Prinzip schon weggekapselt und deine Funktion kann man dank dem Callback auch einfach so verwenden.

Du leitest von TThread ab und rufst dort in execute die Funktion auf. Das Callback ist dabei eine Methode in deiner Thread-Klasse, die das eigentliche Callback (zum Beispiel das Updaten der Progressbar) synchronisiert, dh. im Mainthread, aufruft. Die Parameter für die Funktion kannst du dem Thread im Konstruktor übergeben (der wird noch synchron im Mainthread ausgeführt), für die Rückgabewerte und Exceptions musst du dir noch was überlegen. Ein weiteres synchronisiertes Callback würde sich dafür anbieten.

[mm1256]

Hallo Robert,

danke für den Tipp. Hab's einfach mal probiert....und langsam verliere ich auch die Angst vor den Threads. Weil, es scheint auf Anhieb zu funktionieren.

Für den TE:

Delphi-Quellcode:

unit RuFileCopy;

interface

uses
  System.Classes;

type

  TFileCopyMode = (fomAlways, fomIfNewer, fomIfOlder, fomIfSameDate, fomIfSameDateTime);
  TFileCopyCallback  = procedure(ProgressPercent: Integer) of object;

  TRuFileCopyThread = class(TThread)
  private
    { Private-Deklarationen }
    FCopyResult, FSourceName, FDestName: string;
    FFileCopyMode: TFileCopyMode;
    FFileCopyCallback: TFileCopyCallback;
    procedure ThreadCopyCallback(ProgressPercent: Integer);
  protected
    procedure Execute; override;
  public
    constructor Create(aSourceName, aDestName: string;
                       aFileCopyMode: TFileCopyMode;
                       aFileCopyCallback: TFileCopyCallback);
    procedure ThreadIsReady(Sender: TObject);
  end;

const
  cRuFileCopyReady = 200;

var
  RuFileCopyThreadResult: string = '';

{- Eine Datei kopieren: Direkt-Aufruf -}
function RuFileCopyExecute(aSourceName, aDestName: string;
                           aFileCopyMode: TFileCopyMode;
                           aFileCopyCallback: TFileCopyCallback): string;

{- Eine Datei kopieren: Kopiert die Datei in einem eigenen Thread -}
function RuFileCopyThread(aSourceName, aDestName: string;
                          aFileCopyMode: TFileCopyMode;
                          aFileCopyCallback: TFileCopyCallback): string;

implementation

uses System.SysUtils;

var
  StreamCopyBuffer: array[0..500 * 1024] of byte;
  ProgressPercent: Int64;

{------------------------------------------------------------------------------}
{- Eine Datei kopieren: Direkt-Aufruf oder Aufruf aus dem Thread              -}
{- aSourceName = Name der Quelldatei                                          -}
{- aDestName = Name der ZielDatei, darf auch das Zielverzeichnis sein         -}
{- aFileCopyCallback = ProgressCallback: Übergibt Wert in Prozent von 1-100   -}
{------------------------------------------------------------------------------}
function RuFileCopyExecute(aSourceName, aDestName: string;
                           aFileCopyMode: TFileCopyMode;
                           aFileCopyCallback: TFileCopyCallback): string;
var
  BytesRead, BytesToRead, P: Int64;
  SourceStream, DestStream: TFileStream;
  SourceDateTime, DestDateTime: TDateTime;
begin
  Result := '';
  if not FileExists(aSourceName) then Exit('Quelldatei <'+aSourceName+'> existiert nicht');
  if not FileAge(aSourceName, SourceDateTime) then Exit('Quelldatei: TimeStamp lesen fehlgeschlagen');
  BytesRead := 0;
  ProgressPercent := 0;
  P := 0;
  // DestName darf auch ein Directory sein...dann den Dateinamen ran hängen
  if DirectoryExists(aDestName) // DestName ist ein Directory
  then aDestName := IncludeTrailingBackslash(aDestName)+ExtractFileName(aSourceName);
  if FileExists(aDestName) then begin
    if not FileAge(aDestName, DestDateTime) then Exit('Zieldatei: TimeStamp lesen fehlgeschlagen');
    case aFileCopyMode of
      fomAlways         :;// immer kopieren
      fomIfNewer        : // Die Quelldatei muss neuer als die Zieldatei sein
                          if NOT (SourceDateTime > DestDateTime)
                          then Exit('Die Quelldatei ist nicht neuer als die Zieldatei');
      fomIfOlder        : // Die Quelldatei muss älter als die Zieldatei sein => z.B. Downgrade
                          if NOT (SourceDateTime < DestDateTime)
                          then Exit('Die Quelldatei ist nicht älter als die Zieldatei');
      fomIfSameDate     : // Quell- und Ziel-Datei müssen am selben Tag erstellt worden sein
                          if Trunc(DestDateTime) <> Trunc(SourceDateTime)
                          then Exit('Die Zieldatei ist nicht vom selben Datum als die Quelldatei');
      fomIfSameDateTime : // Das Datum von Quell- und Ziel-Datei muss identisch sein
                          if DestDateTime <> SourceDateTime
                          then Exit('Datum und Zeit von Zieldatei und Quelldatei sind unterschiedlich');
      else raise Exception.Create('Da hat der Programmierer was vergessen');
    end;
  end;
  SourceStream := TFileStream.Create(aSourceName,fmOpenRead or fmShareDenyNone);
  DestStream := TFileStream.Create(aDestName,fmCreate);
  try
    if @aFileCopyCallback = nil
    then DestStream.CopyFrom(SourceStream,SourceStream.Size) // so geht es am schnellsten
    else begin // mit Fortschrittanzeige
      aFileCopyCallback(0);
      BytesToRead := SizeOf(StreamCopyBuffer);
      if SourceStream.Size < BytesToRead then BytesToRead := SourceStream.Size;
      repeat
        SourceStream.ReadBuffer(StreamCopyBuffer,BytesToRead);
        DestStream.WriteBuffer(StreamCopyBuffer,BytesToRead);
        BytesRead := BytesRead + BytesToRead;
        ProgressPercent := (BytesRead * 100) div SourceStream.Size;
        if P <> ProgressPercent then begin
          aFileCopyCallback(Integer(ProgressPercent));
          P := ProgressPercent;
        end;
        if (SourceStream.Size - BytesRead) <= BytesToRead
        then BytesToRead := SourceStream.Size - BytesRead;
      until BytesRead >= SourceStream.Size;
      if BytesRead <> SourceStream.SIZE
      then Result := 'Fehler beim Kopiervorgang:'+#13#10
                     +'Bytes gelesen:' + IntToStr(BytesRead) + #13#10
                     +'Bytes geschrieben:' + IntToStr(SourceStream.SIZE)
      else aFileCopyCallback(100); // Kopiervorgang 100%
    end;
  finally
    SourceStream.Free;
    DestStream.Free;
  end;
end;

function RuFileCopyThread(aSourceName, aDestName: string;
                          aFileCopyMode: TFileCopyMode;
                          aFileCopyCallback: TFileCopyCallback): string;
begin
  with TRuFileCopyThread.Create(aSourceName, aDestName,
                          aFileCopyMode, aFileCopyCallback)
  do begin
    OnTerminate := ThreadIsReady;
    FreeOnTerminate := true;
    Start; // Thread wird gestartet
  end;
end;

{------------------------------------------------------------------------------}
{- TRuFileCopyThread ----------------------------------------------------------}
{------------------------------------------------------------------------------}

procedure TRuFileCopyThread.ThreadCopyCallback(ProgressPercent: Integer);
begin
  Synchronize(procedure
              begin
                FFileCopyCallback(ProgressPercent);
              end);
end;

procedure TRuFileCopyThread.ThreadIsReady(Sender: TObject);
begin
  RuFileCopyThreadResult := FCopyResult;
  Synchronize(procedure
              begin
                FFileCopyCallback(cRuFileCopyReady);
              end);
end;

constructor TRuFileCopyThread.Create(aSourceName, aDestName: string;
  aFileCopyMode: TFileCopyMode; aFileCopyCallback: TFileCopyCallback);
begin
  inherited Create(True); // True = Thread nicht automatisch starten
  FCopyResult := '';
  FSourceName := aSourceName;
  FDestName := aDestName;
  FFileCopyMode := aFileCopyMode;
  FFileCopyCallback := aFileCopyCallback;
  FreeOnTerminate := true;
end;

procedure TRuFileCopyThread.Execute;
begin
  if not Terminated
  then FCopyResult := RuFileCopyExecute(FSourceName, FDestName,
                                        FFileCopyMode, FFileCopyCallback);
end;

end.

// Hauptprogramm

type
  TForm1 = class(TForm)
  ...
  public
    { Public-Deklarationen }
    procedure ShowProgress(ProgressPercent: Integer);
  end;

...

procedure TForm1.ShowProgress(ProgressPercent: Integer);
begin
  ProgressBar1.Position := ProgressPercent;
  if ProgressPercent = cRuFileCopyReady
  then Label4.Caption := 'FERTIG: '+DateTimeToStr(Now)+' Result['+RuFileCopyThreadResult+']';
end;

Man kann während des Kopiervorganges das MainFormular beliebig bewegen, da friert absolut nichts ein und auch der ProgressBar geht schön durch.

ToDo:
- Exception-Behandlung
- Gibt es eine bessere Lösung für die globale Variable "RuFileCopyThreadResult" und die in der Unit lokalen Variablen StreamCopyBuffer und ProgressPercent [EDIT] Stichwort "ThreadSafe", weil man dann mehrere Dateien gleichzeitig kopieren könnte. Das wär's

[Sir Rufo]

@mm1256

Also deine FileCopy-Routine ist echt grausam ... wer hat die denn verbrochen?

Ich wollte die gerade etwas überarbeiten, aber das kommt ja einem neu schreiben gleich.

1. Statt einem String als Rückgabe verwende Exceptions
2. Solche Exceptions raise Exception.Create('Da hat der Programmierer was vergessen'); sind nicht gerade aussagekräftig. Mach es doch richtig:
   
   Delphi-Quellcode:

   case FileCopyMode of
     ...
     else raise ENotImplemented.CreateFmt('FileCopyMode %s nicht implementiert', [GetEnumName( TypeInfo(TFileCopyMode), Ord( FileCopyMode )]);
   end;

   und du bekommst die Fehlermeldung
   

   Zitat:

   FileCopyMode fcmFoo nicht implementiert

3. Statt TStream.ReadBuffer solltest du TStream.Read verwenden, dann bekommst du automatisch die Anzahl der echten gelesenen Bytes zurück. Dann weißt du auch wieviel du schreiben musst
4. Statt einer TFileCopyCallback = procedure(ProgressPercent: Integer) of object; nimm eine TFileCopyCallback = reference to procedure(ProgressPercent: Integer); (optional, weil flexibler)

Wenn du das hast, dann kannst du diese Kopieraktion vom http://www.delphipraxis.net/185335-b...ab-xe2%5D.html aus starten und alles wird gut.

Delphi-Quellcode:

procedure TFoo.FileCopyWorker_DoWork(Sender: TObject; e: TDoWorkEventArgs );
var
  FileCopyArg : TFileCopyArg;
begin
  FileCopyArg := e.Argument.AsType<TFileCopyArg>;
  MyCopyFile( 
    FileCopyArg.SourceName, 
    FileCopyArg.DestName, 
    FileCopyArg.FileCopyMode,
    (Sender as TBackgroundWorker).ReportProgress );
end;

procedure TFoo.FileCopyWorker_ProgressChanged( Sender : TObject; e: TProgressChangedEventArgs );
begin
  ProgressBar1.Position := e.Value;
end;

procedure TFoo.FileCopyWorker_RunWorkerCompleted( Sender: TObject; e: TRunWorkerCompletedEventArgs );
begin
  if Assigned( e.Error ) then
    ShowMessage( e.Error.ToString() )
  else if e.Cancelled then
    ShowMessage( 'Du hast das abgebrochen!' )
  else
    ShowMessage( 'Ich habe fertig!' );
end;

Das ist eigentlich alles und du kannst die originale Routine verwenden.

PS Das Wichtigste hatte ich vergessen:
Du greifst immer wieder auf SourceStream.Size zu, und das macht deine Routine mit Progress-Callback extremst langsam. Denn bei jedem Aufruf wandert der Stream an das Ende liest die Position und kehrt zur vorherigen Position zurück!

Lies den Wert am Anfang einmal aus und merk dir diesen, dann läuft auch die Routine mit dem Progress sehr schnell.

[mm1256]

Zitat von Sir Rufo:

Also deine FileCopy-Routine ist echt grausam ... wer hat die denn verbrochen?

Dieser Verbrecher antwortet gerade

Ich freue mich immer sehr, wenn Verbesserungsvorschläge dazu beitragen, etwas "zu verbessern". Darum habe ich mir deine Vorschläge mal zur Brust genommen:

Zitat von Sir Rufo:

[*]Statt einem String als Rückgabe verwende Exceptions

Mittlerweile kenne ich deinen Standpunkt zur Exception-Behandlung, aber in diesem Fall bringen mir Exceptions anstelle des String-Rückgabewertes (und so hast du das wohl gemeint) überhaupt nicht weiter. Welche Exception soll beispielsweise geworfen werden, wenn die Zieldatei neuer ist? Da ist meiner Meinung nach ein schlichter Rückgabewert geeigneter. Der User soll ja bei Bedarf lediglich darüber informiert werden, was los ist.

Zitat von Sir Rufo:

[*]Solche Exceptions raise Exception.Create('Da hat der Programmierer was vergessen'); sind nicht gerade aussagekräftig. Mach es doch richtig:

Delphi-Quellcode:

case FileCopyMode of
  ...
  else raise ENotImplemented.CreateFmt('FileCopyMode %s nicht implementiert', [GetEnumName( TypeInfo(TFileCopyMode), Ord( FileCopyMode )]);
end;

Gerade um das "richtig machen geht es hier. Diese Exception wird im Normalfall nur im Entwicklungsmodus stattfinden. Der User wird diese niemals zu Gesicht bekommen, denn wenn das passiert, hat der Programmierer bereits Bockmist gebaut und sein Werk nicht mal ansatzweise getestet.

Zitat von Sir Rufo:

PS Das Wichtigste hatte ich vergessen:
Du greifst immer wieder auf SourceStream.Size zu, und das macht deine Routine mit Progress-Callback extremst langsam. Denn bei jedem Aufruf wandert der Stream an das Ende liest die Position und kehrt zur vorherigen Position zurück!

Lies den Wert am Anfang einmal aus und merk dir diesen, dann läuft auch die Routine mit dem Progress sehr schnell.

Das mit dem Getter der Filesize habe ich nicht gewusst. Aber, ich habe es jetzt mal geändert (bin ja nicht beratungsresistent) und mit einer 3,5 GB großen Datei ausprobiert und die Zeiten verglichen. Die Formulierung "extremst langsam" ist total überzogen. Der Unterschied liegt bei dieser 3,5 GB-Datei im Bereich von einigen Millisekunden.

Wenn also Performance raus zu holen ist, dann nicht an dieser Stelle.

[Sir Rufo]

• Zu Exception :
  
  Bei einer Exception wird der Benutzer doch informiert ... es poppt eine MessageBox mit der Meldung auf, ganz von alleine, nur weil du eine Exception wirfst.
  
  Und wenn du mit dem Argument FileCopyMode eine Bedingung festlegst, die dann nicht zutrifft, dann ist das eine Ausnahme. Also Exception werfen und gut.
  
  Die Exception könntest du als
  
  Delphi-Quellcode:

  type
    EFileCopyModeMismatchException = class(Exception);

  deklarieren und entsprechend werfen. Dann kannst du im nachhinein sogar noch speziell darauf reagieren -> Dialog und den Benutzer fragen, ob er evtl. eine anderen FileCopyMode haben möchte.
  
  Und da eine Exception auch nur eine Klasse ist, kannst du da sogar noch alle möglichen Informationen hineinpacken (z.B. den FileCopyMode, der diese Exception verursacht hat).
  
• Zu ENotImplemented :
  
  Wenn dich dein Kunde anruft und dir mitteilt, dass da so eine Fehlermeldung aufgetaucht ist, bei welcher Fehlermeldung wüsstest du sofort worum es geht? - Eben, sehe ich auch so
  
  Ach ja, Fehler passieren immer wieder, auch beim Testen ...
  
• Zu TStream.Size :
  
  Wie stark das bremst hängt natürlich stark von deiner Hardware und wie stark fragmentiert die Datei auf der Platte vorliegt. Im Idealfall sollte man davon nicht wirklich viel merken, im Worst Case kannst du in der Zeit halb Asien neu bevölkern

[Dejan Vu]

Zitat von mm1256:

+...aber in diesem Fall bringen mir Exceptions anstelle des String-Rückgabewertes überhaupt nicht weiter.

Und wie willst Du deine Rückgabefehlermeldungszeichenkette auswerten? Also, woher weißt Du, welcher Fehler genau aufgetreten ist? Ach, das ist Dir egal? Man kann ja streiten, ob Exceptions oder Rückgabewerte besser sind, aber wenn schon Rückgabewerte, dann wenigstens numerisch, oder als Enum.

Stell Dir mal vor, dein Programm soll mal im Ausland laufen...

Übrigens ist es keine Tugend, alles in eine Routine zu quetschen. Das ist sowas von gestern, ach was sag ich, vorvorvorgestern... Definier doch einfach eine Klasse, führe einfache kleine Methoden ein und schreibe das Ganze, das es verständlich ist. "Clean Code" nennt sich das. Probiere es mal aus.

[BUG]

Zitat von Dejan Vu:

Übrigens ist es keine Tugend, alles in eine Routine zu quetschen. Das ist sowas von gestern, ach was sag ich, vorvorvorgestern... Definier doch einfach eine Klasse, führe einfache kleine Methoden ein und schreibe das Ganze, das es verständlich ist. "Clean Code" nennt sich das. Probiere es mal aus.

Prinzipiell bin ich deiner Meinung, aber bei dieser "Begründung" ziehst du niemanden aus dem Schatten der seitenlangen Prozeduren in säubernde Licht des blütenreinen Programmierens

Die Reaktionen sind imho leicht übertrieben (bis auf die zur Fehlerbehandlung ). Es gibt deutlich schlimmeren Code.
Der Hinweis auf den Workerpool ist für diese Aufgabe zwar legitim, man sollte das imho aber auch mal zu Fuß gemacht haben, damit man eine Vorstellung davon hat, was bei so einer asynchronen Ausführung so passiert.

Ein Fehler ist mir ins Auge gefallen: Der RuFileCopyExecute wird in Execute das falsche Callback übergeben. Eigentlich willst du ja die synchronisierte Variante.

Das CreateSuspended = true und das Konstrukt mit dem with kannst du dir sparen, wenn du die Eigenschaften, die du im with setzt, direkt im Konstruktor setzt.
OnTerminate wird im Hauptthread ausgeführt, das synchronize solltest du dir an der Stelle sparen; oder du lässt onTerminate leer und rufst ThreadCopyCallback(cRuFileCopyReady); direkt am Ende von Execute auf.

[Dejan Vu]

Zitat von BUG:

Prinzipiell bin ich deiner Meinung, aber bei dieser "Begründung" ziehst du niemanden aus dem Schatten der seitenlangen Prozeduren in säubernde Licht des blütenreinen Programmierens

Ash on my main.