TIdIOHandler.CheckForDataOnSource: ReadFromSource NoExceptionIfDisconnected (TIdTCPServer)

[VinceALaPlancha]

Hello everyone,

Why is ReadFromSource () inside TIdIOHandler.CheckForDataOnSource being called with ARaiseExceptionIfDisconnected=False?

I have a TIdTCPServer project where my loop looks like this:

1. wait 2000ms for a full packet header (16 bytes) to be available in InputBuffer
2. if header available: read the header + full packet from InputBuffer (+ from socket)
3. process packet if one is received
4. send all packets

However, I sometimes have the situation where a client connection sends an incomplete header (let's say, only 4 bytes of the 16) and then freezes, and this hogs the CPU. The reason seems to be because CheckForDataOnSource(2000) returns immediately (with False) if the client is disconnected but 4 bytes is inside the inputbuffer. I know/think Indy assumes that the connection is still connected when the InputBuffer is not empty. In this situation, ReadFromSource(False,..) is called, but it does not seem to try to read+add data to the buffer, hence does not detect the client connection is gone:

function TIdIOHandler.CheckForDataOnSource(ATimeout: Integer = 0): Boolean;
var
  LPrevSize: Integer;
begin
  Result := False;
  // RLebeau - Connected() might read data into the InputBuffer, thus
  // leaving no data for ReadFromSource() to receive a second time,
  // causing a result of False when it should be True instead.  So we
  // save the current size of the InputBuffer before calling Connected()
  // and then compare it afterwards....
  LPrevSize := InputBuffer.Size;
  if Connected then begin
    // return whether at least 1 byte was received
    Result := (InputBuffer.Size > LPrevSize) or (ReadFromSource(False{WHY FALSE?}, ATimeout, False) > 0);
  end;
end;

I don't really know how to solve this cleanly.

This is a very simplified demo of how my code looks:

type TPacketHeader = record
  x,y,cmd,z: Cardinal
end;

function waitForPacketHeader(aIOHandler: TIdIOHandler; const aMaxTimeMs: LongInt): Boolean;
var
  tStart,msElapsed: Cardinal;
  requiredNofBytes,msLeft: Integer;
  bNewDataReceived: Boolean;
begin
  Result := False;
  requiredNofBytes := SizeOf(TPacketHeader);
  if(aIOHandler.InputBuffer.Size >= requiredNofBytes) then begin
    Result := True;
    exit;
  end;
  if(aMaxTimeMs<=0) then Exit;

  tStart := Ticks();
  msLeft := aMaxTimeMs;
  while(msLeft > 0) do begin
    bNewDataReceived := aIOHandler.CheckForDataOnSource(msLeft);
    if(aIOHandler.InputBuffer.Size >= requiredNofBytes) then begin
      Result := True;
      exit;
    end;
    msElapsed := GetTickDiff(tStart,Ticks());
    if(msElapsed >= MaxInt) then begin
      Exit;
    end;
    msLeft := aMaxTimeMs - msElapsed;
  end;
end;

function parseCmdNrFromInputBuffer(aBuf: TIdBuffer): Cardinal;
var
  c: Cardinal;
  pB: PByte;
begin
  //Result := ... aBuf.ExtractToByte(...);  //just return the cmd as Cardinal
end;


procedure TForm1.IdTCPServer1Execute(AContext: TIdContext);
var
  bPacketHeaderAvailable,b,bHackingAttempt: Boolean;
  incomingCmdNr: Cardinal;
  rcvPacket: TPacket;
begin
  //---1) READ AND PROCESS PACKET (if any received):
  //        wait 2" for a full packetheader.
  //        If a full packetheader is available in the InputBuffer, call readPacket.
  //        Otherwise, proceed and send all packets from queue (if any)
  AContext.Connection.IOHandler.CheckForDisconnect();
  bPacketHeaderAvailable := waitForPacketHeader(AContext.Connection.IOHandler,2000);
  if bPacketHeaderAvailable {AContext.Connection.IOHandler.InputBuffer.Size >= SizeOf(TPacketHeader)} then begin
    incomingCmdNr := parseCmdNrFromInputBuffer(AContext.Connection.IOHandler.InputBuffer);
    bHackingAttempt := (incomingCmdNr>100);  //
    b := False;
    if bHackingAttempt then begin
      //log
      Exit;
    end
    else begin
      //---Read full packet (including the packet header
      b := readPacket(AContext.Connection.Socket,rcvPacket);
      if(b=False) then begin
        //log
      end;
    end;
    //---Process Packet
    if b = True then b := processPacket(AContext, rcvPacket);
  end;

  //---2) SEND ALL PACKETS
  //while (lQueue.Count > 0) do sendPacket(...);
end;

[rlebeau]

Why is ReadFromSource () inside TIdIOHandler.CheckForDataOnSource being called with ARaiseExceptionIfDisconnected=False?

Because, as its name implies, CheckForDataOnSource() is a data check. It returns True if there is new data, otherwise it returns False. Raising an exception is not part of that logic. If you want an exception on disconnect, call CheckForDisconnect() if CheckForDataOnSource() returns False, eg:

if not AContext.Connection.IOHandler.CheckForDataOnSource(2000) then
begin
  AContext.Connection.IOHandler.CheckForDisconnect;
  Continue; // or Exit, as needed...
end;
// process new data as needed...

1. wait 2000ms for a full packet header (16 bytes) to be available in InputBuffer

You know how many bytes you are expecting, so you should just read the full packet header and packet data, and let the reads raise an exception if a disconnect or timeout occurs. For instance, check if the InputBuffer has any bytes first. If it is empty, only then call CheckForDataOnSource() and CheckForDisconnect(). If the InputBuffer is still empty, exit. Otherwise, read the full header and packet and move on.

However, I sometimes have the situation where a client connection sends an incomplete header (let's say, only 4 bytes of the 16) and then freezes, and this hogs the CPU.

The only way it could hog the CPU is if you are using a busy-wait until more bytes arrive, ie such as by calling CheckForDataOnSource() in a tight loop. If you use a sleep-wait instead then it shouldn't use the CPU at all, such as by calling ReadBytes() with a ReadTimeout specified.

The reason seems to be because CheckForDataOnSource(2000) returns immediately (with False) if the client is disconnected

It would return False only if:

• Connected returns False (as you noted, it returns True if there is unread data in the InputBuffer).
• Connected returns True and there was no change in the InputBuffer's size.

Calling CheckForDisconnect() after CheckForDataOnSource() returns False would detect a disconnect.

I know/think Indy assumes that the connection is still connected when the InputBuffer is not empty.

Correct.

In this situation, ReadFromSource(False,..) is called, but it does not seem to try to read+add data to the buffer, hence does not detect the client connection is gone:

The caller is explicitly asking for an exception not be raised. So a disconnect will not raise an exception. however, if the socket is still open then ReadFromSource() will try to read+add. There simply won't be any new data to add, but it will note the disconnect by setting ClosedGracefully=True for later use.

I don't really know how to solve this cleanly.

The clean solution is to not use CheckForDataOnSource() to begin with. Use the normal Read...() methods and let them block until their data arrives, and raise on errors/disconnects. But, if you must use CheckForDataOnSource() then you should use CheckForDisconnect() with it.

This is a very simplified demo of how my code looks:

One thing I notice is that you are calling CheckForDisconnect() before calling CheckForDataOnSource(). You should call it after instead, You can do that inside of waitForPacketHeader().

You are also comparing msElapsed to MaxInt instead of aMaxTimeMs (you are also using the deprecated 32bit ticks functions instead of using the modern 64bit ticks functions).

Try something like this:

type
  TPacketHeader = record
    x, y, cmd, z: UInt32;
  end;

  TPacket = record
    header: TPacketHeader;
    ...
  end;

function waitForPacketHeader(aIOHandler: TIdIOHandler; const aMaxTimeMs: LongInt): Boolean;
const
  requiredNofBytes = SizeOf(TPacketHeader);
var
  tStart: TIdTicks;
  msElapsed: UInt32;
  msLeft: Integer;
begin
  Result := (aIOHandler.InputBuffer.Size >= requiredNofBytes);
  if Result or (aMaxTimeMs <= 0) then begin
    Exit;
  end;
  tStart := Ticks64();
  msLeft := aMaxTimeMs;
  repeat
    if aIOHandler.CheckForDataOnSource(msLeft) then begin
      Result := (aIOHandler.InputBuffer.Size >= requiredNofBytes);
      if Result then begin
        Exit;
      end;
    end else begin
      aIOHandler.CheckForDisconnect;
    end;
    msElapsed := GetElapsedTicks(tStart);
    if (msElapsed >= aMaxTimeMs) then begin
      Exit;
    end;
    msLeft := aMaxTimeMs - msElapsed;
  until (msLeft < 1);
end;

procedure readPacketHeader(aIOHandler: TIdIOHandler; var vPacket: TPacket);
var
  buf: TIdBytes;
begin
  aIOHandler.ReadBytes(buf, SizeOf(TPacketHeader), False);
  Move(buf[0], vPacket.header, SizeOf(TPacketHeader));
  // flip endian of header fields as needed...

  {
  alternatively:

  vPacket.header.x   := aIOHandler.ReadUInt32;
  vPacket.header.y   := aIOHandler.ReadUInt32;
  vPacket.header.cmd := aIOHandler.ReadUInt32;
  vPacket.header.z   := aIOHandler.ReadUInt32;
  }
end;

function readPacketBody(aIOHandler: TIdIOHandler; var vPacket: TPacket): Boolean;
begin
  // use vPacket.header to read body as needed ...
end;

procedure TForm1.IdTCPServer1Execute(AContext: TIdContext);
var
  rcvPacket: TPacket;
begin
  //---1) READ AND PROCESS PACKET (if any received):
  //        wait 2" for a full packetheader.
  //        If a full packet header is available in the InputBuffer, call readPacketBody.
  //        Otherwise, proceed and send all packets from queue (if any)
  if waitForPacketHeader(AContext.Connection.IOHandler, 2000) then begin
    readPacketHeader(AContext.Connection.IOHandler, rcvPacket);
    if (rcvPacket.header.cmd > 100) then begin
      //log and discard rest of full packet
      Exit;
    end;
    //---Read the rest of the packet
    if readPacketBody(AContext.Connection.IOHandler, rcvPacket) then begin
      //---Process Packet
      processPacket(AContext, rcvPacket);
    end else begin
      //log
    end;
  end;

  //---2) SEND ALL PACKETS
  //while (lQueue.Count > 0) do sendPacket(...);
end;

[VinceALaPlancha]

Hello Remy,
thank you for the reply and suggestions.
You are right, I know how many bytes to expect. However, there are some reasons the choice was made (many years ago) to read the header separately from the full packet:

1. main reason was responsiveness: as long as there is no FULL packet header in the InputBuffer, I don't start reading bytes. This means that an incoming packet does not block outgoing messages yet.
2. by reading the header, I can decide faster and easier to disconnect and not start reading the full packet. However, reason nr 1 was the main reason.

The usage of calling IOHandler.CheckForDataOnSource() with a timeout has always been used and kept CPU usage really low, unless data needed to be read/written.
However, recently I started investigating why some connections sometimes consume much CPU cycles: they all have this in common: the client connection gets stuck somewhere (maybe a half open socket) while sending a packet and the TIdTcpServer never detects the disconnection. That's how I ended up writing the waitForPacketHeader() function only recently.

I could rewrite some of the logic as you suggest (I think the conlusion is always reading all data from buffer as soon as there is data in the InputBuffer), but I would prefer to only touch the current logic if there is no other reliable way to detect that non respsonding/disconnected connection.

So in fact only these question remain:

1. can i reliably detect if a client was disconnected if I keep data in the InputBuffer until it reaches a certain size (eg: 16 bytes)? Or is the only reliable way to always read all available data (into an other buffer)?
2. Still a question on the Indy classes: I'm still not convinced why Indy does not detect the disconnect. Eg: in CheckForDisconnect(), it does not detect the situation. But a bit deeper, IdIOHandlerSocket.BindingAllocated() detects it and returns false. I've been able to provoke the situation on my development machine:
   • start the TCP server
   • start the TCP client from Rad Studio, putting a breakpoint where it will send its first 4 bytes

What happens:

• srvExecute() is calling waitForPacketHeader(2000), it returns False each time (OK)
• srvExecute() is calling waitForPacketHeader(2000), it returns False each time (OK)
• ...
• client sends 4 bytes
• srvExecute() is calling waitForPacketHeader(2000):
  • aIOHandler.CheckForDataOnSource returns True (InputBuffer.Size=4) (OK)
  • the waitForPacketHeader() continues calling aIOHandler.CheckForDataOnSource (which now returns False, but still respects the timeout so no CPU hogging) (OK)
• Stop the TCP client while it is in a breakpoint (CTRL-F2 )
• In srvExecute():
  • aIOHandler.CheckForDataOnSource now returns immediately (the timeout is ignored <==)
  • after waitForPacketHeader() finally exits 2000ms later, srvExecute calls CheckForDisconnect, which does nothing.
  • If I debug it, the reason aIOHandler.CheckForDataOnSource() returns False (without "respecting" the timeout or trying to read) is
    • because TIdIOHandler.ReadFromSource() immediately returns False ;
    • because SourceIsAvailable() returns False
    • because TIdIOHandlerSocket.BindingAllocated() returns false.

So somewhere in the code, the disconnect seems detected, but the CheckForDisconnect() doesn't.

Regards,
Vincent

[rlebeau]

1. can i reliably detect if a client was disconnected if I keep data in the InputBuffer until it reaches a certain size (eg: 16 bytes)? Or is the only reliable way to always read all available data (into an other buffer)?

CheckForDisconnect() should be able to handle that, provided CheckForDataOnSource() is detecting/reporting the disconnect in the first place.

2. Still a question on the Indy classes: I'm still not convinced why Indy does not detect the disconnect. Eg: in CheckForDisconnect(), it does not detect the situation. But a bit deeper, IdIOHandlerSocket.BindingAllocated() detects it and returns false.

BindingHandle() returning False means the socket was closed on the server side. That is a condition handled by CheckForDisconnect().

• Stop the TCP client while it is in a breakpoint (CTRL-F2 )

Do you mean terminate the client process? That will certainly sever the TCP connection, but depending on the behavior of the OS, that termination may or may not be detected on the server side right away. A graceful disconnect gets detected right away on the next socket read, but an abnormal disconnect can take time. Keep in mind that TCP is designed to handle temporary network outages and recovery, so until the socket actually times out internally, it may not report any errors at all. The only way Indy knows a client has disconnected is if the socket reports a graceful disconnect or an error. Until that happens, it thinks the connection is still alive.

• In srvExecute():
  • aIOHandler.CheckForDataOnSource now returns immediately (the timeout is ignored <==)

The only way CheckForDataOnSource() ignores the timeout is when either:

• Connected() returns True, and the InputBuffer received new bytes inside of Connected(), thus another read is not needed.
• Connected() returns False, skipping the call to ReadFromSource() that uses the timeout.

If the socket is still open, Connected() performs a socket read with a 0ms timeout to update state information. If it returns False, either the socket has already been closed, or a disconnect/error was reported. Either way, the connection would be considered dead.

• after waitForPacketHeader() finally exits 2000ms later, srvExecute calls CheckForDisconnect, which does nothing.

You said CheckForDataOnSource() returns immediately, so how could waitForPacketHeader() wait the fully 2000ms? Seems contradictory.

CheckForDisconnect() looks for the conditions that caused CheckForDataOnSource() to skip its timeout. But, if the InputBuffer is not empty then the exception is not raised unless you call CheckForDisconnect() with AIgnoreBuffer=True (it's False by default). I suspect this is the crux of your issue. So, try changing aIOHandler.CheckForDisconnect; to aIOHandler.CheckForDisconnect(True, True); instead.

• If I debug it, the reason aIOHandler.CheckForDataOnSource() returns False (without "respecting" the timeout or trying to read) is
  • because TIdIOHandler.ReadFromSource() immediately returns False ;
  • because SourceIsAvailable() returns False
  • because TIdIOHandlerSocket.BindingAllocated() returns false.

That means the socket was closed. CheckForDisconnect() would treat that as a disconnect condition before deciding whether to raise the exception.