How to use Idhttp within TThread Properly?

Question

I currently have this TThread to download some images to a desk, here is the threading Code:

type
  TDownloadUpdateAnimateEvent = procedure(Sender: TObject; AAnimationname: String;
    var AAnimationUrl: String) of object;

type
  TDownloadanimation = class(TThread)
    private
      FOnUpdateAnimate: TDownloadUpdateAnimateEvent;
      FAnimationname : String;
      FAnimationUrl : string;
      FPathImage : String;
      ImageName: string;
      PathURL: string;
      FFileNameImage: string;
    procedure DoUpdateAnimate;
    protected
      procedure Execute; override;
    public
      constructor Create(AAnimationname:string; AAnimationUrl: string; AOnUpdateAnimate : TDownloadUpdateAnimateEvent; APathImage : string);
      property PathImage: string read FPathImage;
      property FileNameImage: string read FFileNameImage;
  end;

{ TDownloadanimation }

constructor TDownloadanimation.Create(AAnimationname, AAnimationUrl: string; AOnUpdateAnimate : TDownloadUpdateAnimateEvent; APathImage : string);
var
  URI: TIdURI;
begin
  inherited Create(false);
  FOnUpdateAnimate := AOnUpdateAnimate;
  FPathImage := APathImage;
  FAnimationname := AAnimationname;
  FAnimationUrl := AAnimationUrl;
  URI := TIdURI.Create(FAnimationUrl);
  try
    ImageName := URI.Document;
    PathURL := URI.path;
  finally
    FreeAndNil(URI);
  end;
end;

procedure TDownloadanimation.DoUpdateAnimate;
begin
  if Assigned(FOnUpdateAnimate) then
    FOnUpdateAnimate(self, FAnimationname, FFileNameImage);
end;

procedure TDownloadanimation.Execute;
var
  aMs: TMemoryStream;
  aIdHttp: TIdHttp;
  IdSSL: TIdSSLIOHandlerSocketOpenSSL;
  path: string;
  dir: string;
  SPEXT : String;
  itsimage: string;
  responsechk: Integer;
begin
  dir := AnsiReplaceText(PathURL, '/', '');

  if (ImageName = '') then
  begin
    exit;
  end;

  path := PathImage + ImageName;

  if fileexists(path) then
  begin
    FFileNameImage := path;
    if Assigned(FOnUpdateAnimate) then
    begin
      Synchronize(DoUpdateAnimate);
    end;
    exit;
  end
  else
    if not fileexists(path) then
    begin
      aMs := TMemoryStream.Create;
      aIdHttp := TIdHttp.Create(nil);
      IdSSL := TIdSSLIOHandlerSocketOpenSSL.Create(nil);
      try
        IdSSL.SSLOptions.Method := sslvTLSv1;
        IdSSL.SSLOptions.Mode := sslmUnassigned;
        aIdHttp.HTTPOptions := [hoForceEncodeParams] + [hoNoProtocolErrorException];
        aIdHttp.IOHandler := IdSSL;
        aIdHttp.AllowCookies := True;
        aIdHttp.Request.UserAgent := 'Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0';
        aIdHttp.HandleRedirects := True;
        aIdHttp.RedirectMaximum := 3;
        try
          aIdHttp.Head(trim(FAnimationUrl));
        except
        end;
        itsimage := aIdHttp.Response.ContentType;
        responsechk := aIdHttp.ResponseCode;

        if responsechk <> 200 then
        begin
          FFileNameImage := 'error';
          if Assigned(FOnUpdateAnimate) then
          begin
            Synchronize(DoUpdateAnimate);
          end;
          exit;
        end;
        if (itsimage = 'image/gif') then
        begin
          try
            aIdHttp.Get(trim(FAnimationUrl), aMs);
          except
          end;
          aMs.SaveToFile(path);
        end;

        try
          if aIdHttp.Connected then
            aIdHttp.Disconnect;
        except
        end; 

     finally
       FreeAndNil(aMs);
       FreeAndNil(IdSSl);
       FreeAndNil(aIdHttp);
     end;
  end;

  FFileNameImage := path;

  if Assigned(FOnUpdateAnimate) then
  begin
    Synchronize(DoUpdateAnimate);
  end;
end;

and here is the Form that call Create Thread

For i := 0 To imageslist.Count-1 do
begin
  Animatref := 'ref';
  Animaturl := imageslist.Strings[i];
  URI := TIdURI.Create(Animaturl);
  try
    ImageName := URI.Document;
  finally
    FreeAndNil(URI);
  end;

  if (ExtractFileExt(ImageName) = '.gif') then
  begin
    if Fileexists(CheckPath+ImageName) then //if image exists then do something and dont start the Thread To download it 
    begin
      //do something
    end 
    else 
    if NOT Fileexists(CheckPath+ImageName) then // not found on desk and start to download
    begin
      addanimation(Animatref, Animaturl);
    end;
  end;
end;


procedure Tform1.addanimation(animationname, animationurl: string);
var
  Pathanimate:string;
begin
  Pathanimate := appfolder;
  Downloadanimation := TDownloadanimation.Create(animationname, animationurl,  UpdateAnimate, Pathanimate);
end;

And when the application Destroy I call:

if Assigned(Downloadanimation) then
begin
  Downloadanimation.Terminate;
  FreeAndNil(Downloadanimation);
end;

But I got run time error each time I close the application after downloading Thread Fired. Is this because i download multiple images at the same time? if so is there better way to write the thread like waiting after the download image finish then start new download if this was the real issue .

Answer 1

After you signal the thread to terminate, call WaitFor() on it before freeing it:

if Assigned(Downloadanimation) then
begin
  Downloadanimation.Terminate;
  Downloadanimation.WaitFor; // <-- add this
  FreeAndNil(Downloadanimation);
end;

Also, your Form's logic has the potential of running multiple download threads at the same time, but you are only keeping track of the last thread that is created. And you are not freeing it when it is finished, only when the app exits. You should either set FreeOnTerminate=True on each thread, or else store all of the threads in a TList and use their OnTerminated event to know when each thread is finished so you can remove it from the list and free it.

On a separate note, your thread's Execute() logic can reduce its network traffic a little by removing the call to TIdHTTP.Head() altogether, and instead set the TIdHTTP.Request.Accept property to 'image/gif' when calling TIdHTTP.Get(). The server should report a 406 Not Acceptable response if the requested resource exists but is not a GIF, anything else would report an HTTP error, just like TIdHTTP.Head() would have. There is no point in sending a HEAD request in this example.