Deadlock on tidtcpserver.active = false (Delphi2007/indy10)

Question

I am having a problem with some code that was written by a developer that has now left our company, the code implements a tcpserver that responds to an XML based protocol. This appears to be working absolutely fine in our test environment but one or two customers are having problems with application shutdown.

I have traced this to what appears to be a deadlock when tidtcpserver.active = false is called. I am already aware that a deadlock can be caused by one of the connection treads making a synchronised call to the main thread, whilst the main thread is waiting for the connection threads to terminate.

I am already using a tidthreadsafestringlist to pass the data to the main thread for processing, and where I need to call a procedure from the main thread I have created a tidnotify descendant to do this. can anyone think of anything else to look for.

Answer 1

Deadlocks when setting the Active property to False are a classic, and often discussed, issue when using TIdTCPServer incorrectly. The only way the server deadlocks is when the connection threads do not terminate correctly. That MAY be caused by synchronizing operaton to the main thread while the main thread is busy deactivating the server, in which case the use of TIdNotify would does eliminate any such deadlock conditions.

However, that is not the only way to prevent a connection thread from terminating. Another possibility is if your server event handlers have exception handling in them that is blocking Indy's internal notifications from being processed. That can cause runaway threads that keep running but never know that they need to stop. If you are catching exceptions in your code, make sure you re-throw any EIdException-derived exceptions and let the server handle them internally.

Answer 2

I had already been checking the exception handling,

this is what i have in the onexecute event

try
  // code to handle connection including tidnotify etc....
except
  on E:Exception do
  begin
    if (e.InheritsFrom(EIdSilentException) = False) then
      TXMLTraceNotify.XMLTrace('TCPServerExecute: ' + E.Message,ttProblem, FTraceProc);
    raise; //we must raise all exceptions for indy to handle them.
  end;

end;

Answer 3

Here is the complete onExecute Event Handler. I have no onConnect or onDisconnect Event handlers. I have just spotted that if the creation of the Stringlist fails with an exception, the context would not get disconnected. I may change this to use 2 try finally sections.

The various response methods contain some complicated code but they make use of tidnotify and threadsafestringlists.

What I cant understand is why I don't get the problem here even when I configure our software with the exact same settings and use the same hardware to send the XML.

There are also many customers using this successfully with as many as 50-60 terminals communicating fine and no deadlocks. The 2 systems which experience these issues are only responding to 2 terminals. We have power cycled the terminals and also put the latest firmware in them etc..... not sure if its the code now, but i cant see any other explanation for the apparent deadlock.

procedure TCustomXMLServer.TCPServerExecute(aContext: TIdContext);
// MAIN sever execution routine. Called when stuff comes in.
var
  RequestStr: string;
  inList : TStringList;
begin

  with aContext.Connection.IOHandler do
  try

    inList := TStringList.Create;
    try
      ReadTimeout := TimeOut;
      WriteLn ('<srv><connect>ELF Comms Server</connect></srv>');
      repeat
        RequestStr := ReadLn(EOL);
        if RequestStr <> '' then
          InList.add(RequestStr);
        if Pos('</interface>' , RequestStr) > 0 then
          Break;
      // Read all the message data into the InList string list.
      until (readLnTimedout);

      {if READER_SHOW_DEBUG then
        DisplayStrings(InList);}


       // Check for a heartbeat.
      if TElfUtils.IsSubStrInString('<heartbeat>', inList.Text)then
        HeartBeatResp(aContext.Connection.IOHandler, inList.Text)

      // Check for a enquiry.
      else if TElfUtils.IsSubStrInString('<enq>', inList.Text)then
        EnquiryResp(aContext.Connection.IOHandler, inList.Text)

      // Check for an transaction.
      else if TElfUtils.IsSubStrInString('<trans>', inList.Text) then
        TransResp(aContext.Connection.IOHandler, inList.Text)

     // Check for an validation request.
      else if TElfUtils.IsSubStrInString('<val>', inList.Text) then
        ValidationResp(aContext.Connection.IOHandler, inList.Text)

      // Check for download request
      else if TElfUtils.IsSubStrInString('<requestDownload>', inList.Text) then
        DownloadResp(aContext.Connection.IOHandler, inList.Text)

      //Reciving File From device NEW!
      else if TElfUtils.IsSubStrInString('<uploadFile>', inList.Text) then
        UploadResp(aContext.Connection.IOHandler, inList.Text)

      // Check for download request NEW!
      else if TElfUtils.IsSubStrInString('<requestDownloadFile>', inList.Text) then
        DownloadResp(aContext.Connection.IOHandler, inList.Text);

    finally
      inList.Free;
      aContext.Connection.Disconnect;
    end;

  except
    on E:Exception do
      begin
        if not (e is EIdSilentException) then
          TXMLTraceNotify.XMLTrace('TCPServerExecute: ' + E.Message, ttProblem, FTraceProc);
        raise; //we must raise all exceptions for indy to handle them.
      end;
  end;

end;

Answer 4

Here is how i'm using the TS-stringlist

Declaration.

public
  TransactionStrings: TIdThreadSafeStringList;

its created in the constructor and destroyed in the destructor.

this is how i'm adding to it in the context of the tcpserver.

  TransactionStrings.Add(newTrans.AsString);

And this is how i'm reading from it in the context of the main application thread

slXMLTrans := TStringList.Create;
try
  slTemp := FCustomXMLServer.TransactionStrings.Lock;
  try
    slXMLTrans.Assign(slTemp);
    slTemp.Clear;
  finally
    FCustomXMLServer.TransactionStrings.Unlock;
  end;

  if slXMLTrans.Count > 0 then
  begin
    for i := 0 to Pred(slXMLTrans.Count) do
      TAbstractTerminal.ProcessXMLTrans(slXMLTrans[i]);
    slXMLTrans.Clear;
  end;
finally
  slXMLTrans.Free;
end;

I think this is the correct way to use it but I await your comments.