There are two major problems with your TCP reading code:
You are not ensuring that the
InputBuffer
actually hasMsgSize
number of bytes available before callingExtractToBytes()
the second time. If you try to extract more bytes than are actually in the buffer,ExtractToBytes()
raises an exception.more importantly, you are not resizing your
Buffer
variable back to 0 before each call toExtractToBytes()
. After the first call in the first loop iteration, the length of theBuffer
is 4 bytes. If that message is less than 4 bytes in size, you are leaving behind random bytes at the end of yourBuffer
that are being passed on to your parser and likely corrupts its logic. But worse, if there is another message size in the buffer, your next loop iteration makes a 3rd call toExtractToBytes()
and appends those 4 bytes to the end of the existingBuffer
content, not replaces the content like you are assuming (theAAppend
parameter ofExtractToBytes()
is True by default). Thus, you end up copying 4 bytes from the previous message data into yourMsgSize
variable instead of the new 4 bytes you just extracted, so you are using a corruptedMsgSize
value on the nextExtractToBytes()
call.
Because your packets are length-prefixed, you do not need to use CheckForDataOnSource()
or access the InputBuffer
directly at all. Use the following code and let Indy do the work for you:
procedure TForm1.THEX_TCP;
var
Buffer : TBytes;
MsgSize : Integer;
begin
MsgSize := TCPClient.IOHandler.ReadLongInt;
TCPClient.IOHandler.ReadBytes(Buffer, MsgSize);
Inc(fRXCount);
NAT.RecievedNATData(Buffer);
end;
By default, that will block the caller until data is available to read. If THEX_TCP
needs to exit when there is no data ready to be read, use something like this instead:
procedure TForm1.THEX_TCP;
var
Buffer : TBytes;
MsgSize : Integer;
begin
if TCPClient.IOHandler.InputBufferIsEmpty then
begin
TCPClient.IOHandler.CheckForDataOnSource;
TCPClient.IOHandler.CheckForDisconnect;
if TCPClient.IOHandler.InputBufferIsEmpty then Exit;
end;
repeat
MsgSize := TCPClient.IOHandler.ReadLongInt;
TCPClient.IOHandler.ReadBytes(Buffer, MsgSize);
Inc(fRXCount);
NAT.RecievedNATData(Buffer);
SetLength(Buffer, 0);
until TCPClient.IOHandler.InputBufferIsEmpty;
end;
The only gotcha with this approach is that ReadLongInt()
and ReadBytes()
might read more bytes into the InputBuffer
, so your loop could potentially running for a long time if a lot of data is being sent in a short amount of time. If you absolutely must read only one buffer at a time and only process complete messages then use something like this:
procedure TForm1.THEX_TCP;
var
MsgSizeBuffer: array[0..3] of Byte;
MsgSize, I : Integer;
Buffer : TBytes;
begin
TCPClient.IOHandler.CheckForDataOnSource;
TCPClient.IOHandler.CheckForDisconnect;
while TCPClient.IOHandler.InputBuffer.Size >= 4 do
begin
// unfortunately, TIdBuffer does not have a way to peek
// multiple bytes at a time without removing them
for I := 0 to 3 do
MsgSizeBuffer[I] := TCPClient.IOHandler.InputBuffer.Peek(I);
Move(MsgSizeBuffer[0], MsgSize, 4);
MsgSize := LongInt(GStack.NetworkToHost(LongWord(MsgSize)));
if TCPClient.IOHandler.InputBuffer.Size < (4+MsgSize) then
Break;
TCPClient.IOHandler.InputBuffer.Remove(4);
TCPClient.IOHandler.InputBuffer.ExtractToBytes(Buffer, MsgSize);
Inc(fRXCount);
NAT.RecievedNATData(Buffer);
SetLength(Buffer, 0);
end;
end;