Proper way to free and reuse a thread

Question

I'm using Delphi XE2 and my application is used to notify about new records in twitter/rss. In my application I use 2 threads to grab some data from twitter & rss every second.

Here is the code:

type section:

  TWarframeEvent=record
    GUID: String;
    EventType: Byte; // 0 = unknown; 1 = alert; 2 = invasion; 3 = infestation
    Planet: String;
    Mission: String;
    EventDate: TDateTime;
    Time: Integer;
    RewardCredits: LongWord;
    RewardOther: String;
    RewardOtherAmount: Integer;
    Notified: Boolean;
    ItemIndex: Integer;
    Hidden: Boolean;
  end;

  TWarframeNotifyEvent=record
    NotifyTimeLeft: LongWord;
    ID: Integer;
    FlashOnTaskbar: Boolean;
    PlaySound: Boolean;
    Volume: Integer;
    TrayPopupBalloon: Boolean;
  end;

  TWarframeEventList=record
    WarframeEvent: Array of TWarframeEvent;
    WarframeEventCount: Integer;
    NotifyEvent: TWarframeNotifyEvent;
  end;


  TUpdateFromTwitterThread=class(TThread)
    TwitterURL: String;
    Procedure Execute; override;
  end;

  TUpdateFromRSSThread=class(TThread)
    RSS_URL: String;
    Procedure Execute; override;
  end;

var section (module)

  WarframeEventList: TWarframeEventList;

implementation section:

procedure TForm1.TimerUpdateEventsTimer(Sender: TObject);
begin
  UpdateFromTwitterThread:=TUpdateFromTwitterThread.Create(True);
  UpdateFromTwitterThread.TwitterURL:=form2.EditAlertsURL.Text;
  UpdateFromTwitterThread.Start;
  UpdateFromRSSThread:=TUpdateFromRSSThread.Create(True);
  UpdateFromRSSThread.RSS_URL:=form2.EditInvansionsURL.Text;
  UpdateFromRSSThread.Start;
end;

procedure TUpdateFromTwitterThread.Execute;
var
  HTTPClient: TIdHTTP;
  IOHandler: TIdSSLIOHandlerSocketOpenSSL;
  S, S2: String;
  i, l: Integer;
  NewAlertDate: TDateTime;
  ErrorLogFile: TextFile;
begin
  HTTPClient:=TIdHTTP.Create(nil);
  IOHandler:=TIdSSLIOHandlerSocketOpenSSL.Create(nil);
  try
    try
      HTTPClient.IOHandler:=IOHandler;
      HTTPClient.HandleRedirects:=True;
      S:=HTTPClient.Get(self.TwitterURL);
    except
      Assign(ErrorLogFile, AppPath+'Error.log');
      if not(FileExists(AppPath+'Error.log')) then
        Rewrite(ErrorLogFile);
      Append(ErrorLogFile);
      Writeln(ErrorLogFile, DateTimeToStr(Now)+' '+LS_ErrorConnection+' '+self.TwitterURL+'; Error code: '+IntToStr(HTTPClient.ResponseCode));
      Close(ErrorLogFile);
      self.Terminate;
      HTTPClient.Free;
      IOHandler.Free;
    end;
  finally
    HTTPClient.Free;
    IOHandler.Free;
  end;

  if Application.Terminated or self.Terminated then exit;

  S:=copy(S, pos('<b>WarframeAlerts</b></span>', S), Length(S)-pos('<b>WarframeAlerts</b></span>', S));

  while pos('tweet-timestamp js-permalink js-nav', S)>0 do begin
    S:=copy(S, pos('tweet-timestamp js-permalink js-nav', S)+35, Length(S)-pos('tweet-timestamp js-permalink js-nav', S)-35);
    S2:=copy(S, pos('data-time="', S)+11, Length(S)-pos('data-time="', S)-11);
    NewAlertDate:=StrToInt(copy(S2, 1, pos('"', S2)-1));
    S2:=copy(S, pos('<p class="js-tweet-text tweet-text">', S)+36, pos('</p>', S)-pos('<p class="js-tweet-text tweet-text">', S)-36);
    for i:= 0 to WarframeEventList.WarframeEventCount-1 do
      if (WarframeEventList.WarframeEvent[i].EventDate=NewAlertDate) and (WarframeEventList.WarframeEvent[i].Planet=copy(S2, 1, pos(')', S2))) then
        NewAlertDate:=0;

    if NewAlertDate=0 then continue;

    Inc(WarframeEventList.WarframeEventCount);
    SetLength(WarframeEventList.WarframeEvent, WarframeEventList.WarframeEventCount);

    for i:= 0 to WarframeEventList.WarframeEventCount-2 do begin
      if WarframeEventList.WarframeEvent[i].EventDate>NewAlertDate then begin
        for l:=WarframeEventList.WarframeEventCount-1 downto i+1 do
          WarframeEventList.WarframeEvent[l]:=WarframeEventList.WarframeEvent[l-1];
        Break;
      end;
    end;

    if i<=WarframeEventList.NotifyEvent.ID then Inc(WarframeEventList.NotifyEvent.ID);

    WarframeEventList.WarframeEvent[i].GUID:='';
    WarframeEventList.WarframeEvent[i].ItemIndex:=-2;
    WarframeEventList.WarframeEvent[i].EventType:=1;
    WarframeEventList.WarframeEvent[i].Planet:=copy(S2, 1, pos(')', S2));
    S2:=copy(S2, Length(WarframeEventList.WarframeEvent[i].Planet)+3, Length(S2)-Length(WarframeEventList.WarframeEvent[i].Planet));
    WarframeEventList.WarframeEvent[i].Mission:=copy(S2, 1, pos(' -', S2)-1);
    WarframeEventList.WarframeEvent[i].EventDate:=NewAlertDate;
    S2:=copy(S2, Length(WarframeEventList.WarframeEvent[i].Mission)+4, Length(S2)-Length(WarframeEventList.WarframeEvent[i].Mission));
    WarframeEventList.WarframeEvent[i].Time:=StrToInt(copy(S2, 1, pos('m', S2)-1))-1;
    S2:=copy(S2, pos('-', S2)+2, Length(S2)-pos('-', S2));
    WarframeEventList.WarframeEvent[i].RewardCredits:=StrToInt(copy(S2, 1, pos('cr', S2)-1));
    WarframeEventList.WarframeEvent[i].RewardOther:=copy(S2, pos('cr', S2)+5, Length(S2)-pos('cr', S2));
    WarframeEventList.WarframeEvent[i].RewardOtherAmount:=1;
    WarframeEventList.WarframeEvent[i].Notified:=False;
    WarframeEventList.WarframeEvent[i].Hidden:=False;
  end;

  self.Free;
end;

procedure TUpdateFromRSSThread.Execute;
var
  HTTPClient: TIdHTTP;
  IOHandler: TIdSSLIOHandlerSocketOpenSSL;
  S, S2, S3: String;
  i: Integer;
  NewEventType: Byte;
  ErrorLogFile: TextFile;
begin
  HTTPClient:=TIdHTTP.Create(nil);
  IOHandler:=TIdSSLIOHandlerSocketOpenSSL.Create(nil);
  try
    try
      HTTPClient.IOHandler:=IOHandler;
      HTTPClient.HandleRedirects:=True;
      S:=HTTPClient.Get(self.RSS_URL);
    except
      Assign(ErrorLogFile, AppPath+'Error.log');
      if not(FileExists(AppPath+'Error.log')) then
        Rewrite(ErrorLogFile);
      Append(ErrorLogFile);
      Writeln(ErrorLogFile, DateTimeToStr(Now)+' '+LS_ErrorConnection+' '+self.RSS_URL+'; Error code: '+IntToStr(HTTPClient.ResponseCode));
      Close(ErrorLogFile);
      self.Terminate;
      HTTPClient.Free;
      IOHandler.Free;
    end;
  finally
    HTTPClient.Free;
    IOHandler.Free;
  end;

  if Application.Terminated or self.Terminated then exit;

    for i:= 0 to WarframeEventList.WarframeEventCount-1 do
      if (WarframeEventList.WarframeEvent[i].EventType=2) or (WarframeEventList.WarframeEvent[i].EventType=3) then
        WarframeEventList.WarframeEvent[i].Time:=0;

  while pos('<item>', S)>0 do begin
    S:=copy(S, pos('<item>', S)+6, Length(S)-pos('<item>', S)-6);
    S2:=LowerCase(copy(S, pos('<author>', S)+8, pos('</author>', S)-pos('<author>', S)-8));
    NewEventType:=0;
    if S2='alert' then
      NewEventType:=1;
    if S2='invasion' then
      NewEventType:=2;
    if S2='outbreak' then
      NewEventType:=3;

    if NewEventType=1 then
      Continue;

    S2:=LowerCase(copy(S, pos('<guid>', S)+6, pos('</guid>', S)-pos('<guid>', S)-6));
    for i:= 0 to WarframeEventList.WarframeEventCount-1 do
      if ((WarframeEventList.WarframeEvent[i].EventType=2) or (WarframeEventList.WarframeEvent[i].EventType=3)) and (WarframeEventList.WarframeEvent[i].GUID=S2) then begin
        WarframeEventList.WarframeEvent[i].Time:=1;
        NewEventType:=255;
      end;

    if NewEventType=255 then
      Continue;

    Inc(WarframeEventList.WarframeEventCount);
    SetLength(WarframeEventList.WarframeEvent, WarframeEventList.WarframeEventCount);

    WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].ItemIndex:=-2;
    WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].GUID:=S2;
    WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].EventType:=NewEventType;
    WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].Time:=1;
    WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].RewardOther:='';
    WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].RewardOtherAmount:=0;
    WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].RewardCredits:=0;
    S2:=copy(S, pos('<title>', S)+7, pos('</title>', S)-pos('<title>', S)-7);
    if NewEventType=2 then begin
      S2:=Copy(S2, 1, pos(' - ', S2)-1);
      WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].RewardOther:=S2;
      S3:=Copy(S2, 1, pos('VS.', S2)-1);
      S3:=Copy(S3, pos('(', S3), Length(S3)-pos('(', S3)+1);
      if pos('x ', S3)>0 then
        WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].RewardOtherAmount:=StrToInt(copy(S3, 2, pos('x ', S3)-2));
      if pos('K)', S3)>0 then
        WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].RewardCredits:=StrToInt(copy(S3, 2, pos('K)', S3)-2))*1000;
      S3:=Copy(S2, pos('VS.', S2)+4, Length(S2)-pos('VS.', S2)-3);
      S3:=Copy(S3, pos('(', S3), Length(S3)-pos('(', S3)+1);
      if pos('x ', S3)>0 then
        if WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].RewardOtherAmount<StrToInt(copy(S3, 2, pos('x ', S3)-2)) then
          WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].RewardOtherAmount:=StrToInt(copy(S3, 2, pos('x ', S3)-2));
      if pos('K)', S3)>0 then
        if WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].RewardCredits<StrToInt(copy(S3, 2, pos('K)', S3)-2))*1000 then
          WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].RewardCredits:=StrToInt(copy(S3, 2, pos('K)', S3)-2))*1000;
    end;
    if NewEventType=3 then begin
      S2:=Copy(S2, 1, pos(' - ', S2)-1);
      WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].RewardOther:='';
      if pos('x ', S2)>0 then begin
        WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].RewardOtherAmount:=StrToInt(copy(S2, 1, pos('x ', S2)-1));
        WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].RewardOther:=S2;
      end;
      if copy(S2, Length(S2), 1)='K' then
        WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].RewardCredits:=StrToInt(copy(S2, 1, Length(S2)-1))*1000;
      S2:=copy(S2, 1, Length(S2)-1);
    end;
    S2:=copy(S, pos('<title>', S)+7, pos('</title>', S)-pos('<title>', S)-7);
    WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].Planet:=Copy(S2, pos(' - ', S2)+3, Length(S2)-pos(' - ', S2));
    WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].Mission:=copy(S, pos('<author>', S)+8, pos('</author>', S)-pos('<author>', S)-8);
    WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].Notified:=False;
    WarframeEventList.WarframeEvent[WarframeEventList.WarframeEventCount-1].Hidden:=False;
  end;

  self.Free;
end;

The questions are:

1) Is there a way to re-use threads without destroying/re-creating them every minute? Needless to say, this causes memory fragmentation and programs starts to bloat after a day of non-stop work. I do not ask how to make a looped thread since they are not applicable here.

2) When is the right time to free threads? Should I free it before they finish their work or before I create them in timer handler (the one which fires every minute)? Or maybe thread suicides after execute is finished?

3) Should I make

HTTPClient: TIdHTTP;
IOHandler: TIdSSLIOHandlerSocketOpenSSL;

an external (global) objects to avoid destroying/re-creating them every minute?

4) When I try to close my application, it deadlocks if threads were active. Is there any way to terminate threads even if they didn't finish their work or maybe detach them?

Answer 1

You've asked a lot of questions here, and then added a large amount of code. The code is very badly broken. I can see:

• Erroneous call to Self.Free from inside Execute. That is simply wrong. You must use FreeOnTerminate if you want a self-destroying thread.
• You have double free of HTTPClient and IOHandler in case of exception.
• There appears to be a dangerous data race in the list that you mutate in those two threads.

I am sure that there are many other errors in your code.

But I don't want to get into that. And I don't want to address all four questions that you asked directly. I will restrict myself to one only. Deal with that and all other problems go away I believe.

Is there a way to re-use threads without destroying/re-creating them every minute?

In the comments you also say:

What if I don't free it after exception and call Start every minute?

This seems to me to indicate the fundamental misunderstanding that you have. A thread's code is nothing more than a single function call. In the case of TThread that function is Execute. When the thread starts, Execute is called. When Execute returns, that thread's useful life is over. It cannot be restarted once Execute returns.

What this means is that if you want a single thread to perform multiple repetitions of a single task, then you need to implement a loop inside the Execute method.

You also state that:

I do not ask how to make a looped thread since they are not applicable here.

I'm sorry, but that is not correct. The way that you make a thread perform a task multiple times is by looping.

---

More generally I feel that you would be well served by availing yourself of a higher level parallel library. The best available is OTL.

If you do not wish to take that advice then here is how I would go about structuring your program:

1. Remove the timer.
2. Create two threads, and have them looping indefinitely until terminated. That's a standard while not Terminated loop in the Execute method.
3. Have the threads do their work in each iteration of the loop.
4. When they are done working, they need to wait for a minute (or however long). Do that by waiting on an event for a specified timeout.
5. That event can be known to the controlling master thread which uses the event to cancel the threads when it is time to quit.
6. So in order to quit the master thread calls Terminate on the two threads, sets the cancel event, and calls Free on the two threads. Then the application is safe to terminate.

If you architect your program like this all the other questions go away. You only create two threads for the entire life of the program. That deals with questions 1, 2 and 4. As for question 3, you keep those objects as local variables. They are local to the thread and should be locals under Execute. Or better still, locals in helper methods that Execute calls. Your Execute methods are very large.

Separate to this, is the data race on the list. I cannot give you detailed advice on how to fix that. Clearly some serialization is called for.

Answer 2

1. the easiest way to re-use the thread is to run a loop within Execute (instead of executing the operation only once and then terminate), and checking while not Terminated or waiting for an event in the loop to allow clean shutdown
2. free the threads when you no longer need them: you can either use Terminate or a signal to indicate that they should end their work. If FreeOnTerminateis True, there is no need for additional termination and cleanup code in the main thread
3. no, local (private) variables within the thread are better because the thread should have as little external dependencies as possible
4. there can be many reasons for deadlocks, but the risk of deadlocks is reduced if you can make them fully independent from "outer resources"