Crashing listening thread for TIdTCPClient

[Diaboli]

System: Dell M1710 (2,16GHz Duo| 2048mb RAM | GeForce Go 7950 GTX 512mb)
IDE: Delphi 2006
Library: Indy 10

I have encountered an irritating problem with the thread that i made to listen for incomming data from my TIdTCPClient.

[pascal]
type
TListenThread = class(TThread)
Crit: TCriticalSection;

procedure ClientReceive(Rec: String);

procedure Execute; override;

constructor Create;
end;

var
Client: TIdTCPClient;
IOHandler: TIdIOHandlerStack;

implementation

{ TListenThread }

procedure TListenThread.ClientReceive(Rec: String);
var
OpCode: TOpCode;
begin
if (Length(Rec) < 2) then Exit;

OpCode[0]:= Rec[1]; //Extract OpCode
OpCode[1]:= Rec[2]; //
Delete(Rec,1,2); //Remove OpCode from Packet Data
HandlerThread.AddPacket(OpCode, Rec);
end;

constructor TListenThread.Create;
begin
inherited Create(False);
end;

procedure TListenThread.Execute;
begin
while (not Terminated) do
begin
if (Client.Connected) then
ClientReceive(Client.IOHandler.ReadLn);
end;
end;
[/pascal]

The problem occurs when i try to send a packet, using another thread. the other thread is using a TCriticalSection when sending, so that it should not "collide", but it does. Sometimes when i send data, the listenthread terminates.

[Diaboli]

More info:

This happens when the client receives my "SMSG_CharEnum" packet (wich contains the characters that are registerred to the account).

When the client receives an "Login OK" flag, it requests the CharEnum. and when it comes, it crashed the listening thread.

I made the client respond to a "Login OK" by sending a login packet insted, and it didnt crash.

Conclusion: The content of the CharEnum packet causes the error. I guess that somewhere in the CharEnum Packet, there is a byte(or bytes) that match the newline-command. wich causes the ReadLn command to flip.

Any ideas?

I manually deleted the accounts character, and it did not crash when receiving the packet.

[Brainer]

Ehm... Where do you initialize/deinitialize and use critical sections? :| I can't see this in your snippet.

[Diaboli]

I use the critsections in the other threads that uses "Client", for instance the thread that sends packets that are in the outgoing que:
[pascal]
procedure TConnectionthread.SendPacket(Packet: TPacket);
begin
Crit.Acquire;
try
if (Client = nil) then Exit;

if Client.Connected then
begin
Client.IOHandler.WriteLn(Packet.OpCode[0]+Packet.OpCode[1]+Packet.Data);
end
else
begin
Controller.SocketStatus:= 'Not Connected!';
Client.Connect;
end;
finally
Crit.Release;
end;
end;
[/pascal]

i tried putting critsections in the listeningthread too, but it made no difference... :/

[Diaboli]

Now this is possitively puzzling:

When there are no characters assosiated with the account, the packet is received and everything "works".

When there is a character assosiated with the account, the client actually crashes BEFORE the servers sends the responce.

ARGH!

EDIT: Finally found out that it has something to do with my custom ConvertUtils unit, wich i use to put and extract different types from strings.
(IntToRaw(Int: Integer) for instace, returns a 4-byte string, containing the integer)

i have recently redone it totally, and i am sad to say i must have messed it up quite badly...

here is the source for the functions:

[pascal]
Function StrToRaw(Str: String): String;
begin
Result:= IntToHex(Length(Str),2);
Result:= HexToStr(Result);
Result:= Result+Str;
end;

Function BoolToRaw(Bool: Boolean): String;
begin
if Bool then Result:= IntToHex(255,2)
else Result:= IntToHex(0,2);
Result:= HexToStr(Result);
end;

Function ByteToRaw(Byt: Byte): String;
begin
Result:= Chr(Byt);
end;

Function IntToRaw(Integr: Integer): String;
var
PS: PString;
begin
PS:= PString(@Integr);
Result:= PS^;
end;

Function Int64ToRaw(Integr: Int64): String;
var
PS: PString;
begin
PS:= PString(@Integr);
Result:= PS^;
end;

Function SingleToRaw(Singl: Single): String;
var
PS: PString;
begin
PS:= PString(@Singl);
Result:= PS^;
end;

Function GetInt(var RawStr: String): Integer;
var
IntStr: String;
PI: PInteger;
begin
IntStr:= LeftStr(RawStr,4);
Delete(RawStr,1,4);
PI:= PInteger(@IntStr);
Result:= PI^;
end;

Function GetInt64(var RawStr: String): Int64;
var
IntStr: String;
PI: PInt64;
begin
IntStr:= LeftStr(RawStr,;
Delete(RawStr,1,;
PI:= PInt64(@IntStr);
Result:= PI^;
end;

Function GetSingle(var RawStr: String): Single;
var
SinglStr: String;
ps: PSingle;
begin
SinglStr:= LeftStr(RawStr,4);
Delete(RawStr,1,4);
PS:= PSingle(@SinglStr);
Result:= PS^;
end;

Function GetByte(var RawStr: String): Byte;
var
PB: PByte;
ByteStr: String;
begin
ByteStr:= LeftStr(RawStr,1);
Delete(RawStr,1,1);
PB:= PByte(@ByteStr);
Result:= PB^;
end;

Function GetStr(var RawStr: String): String;
var
LenByte: Byte;
IntStr: String;
begin
IntStr:= StrToHex(RawStr[1]);
Delete(RawStr,1,1);
LenByte:= HexToInt(IntStr);
Result:= LeftStr(RawStr,LenByte);
Delete(RawStr,1,LenByte);
end;

Function GetBool(var RawStr: String): Boolean;
var
BoolByte: Byte;
begin
Result:= False;
BoolByte:= GetByte(RawStr);
if BoolByte = 255 then Result:= True;
end;
[/pascal]

[Brainer]

I'm a little worried about how you use and initialize your critical sections. In my humble opinion, you should do it a little different. Here is a little example.

[pascal]
{

This is an example from the book entitled
"Delphi 6 Developer's Guide".

}

unit Unit1;

interface

uses
Windows, Messages, SysUtils, Variants, Classes, Graphics, Controls, Forms,
Dialogs, StdCtrls;

const
ArrSize = 128;

type
{ .: TFooThread :. }
TFooThread = class(TThread)
protected
{ Protected declarations }
procedure Execute(); override;
end;

{ .: TForm1 :. }
TForm1 = class(TForm)
ListBox1: TListBox;
Button1: TButton;
procedure Button1Click(Sender: TObject);
private
{ Private declarations }
procedure MyThreadTerminate(Sender: TObject);
public
{ Public declarations }
end;

var
Form1: TForm1;
NextNumber: Integer = 0;
DoneFlags: Integer = 0;
GlobalArray: array[1..ArrSize] of Integer;
CS: TRTLCriticalSection;

implementation

{$R *.dfm}

{ .: GetNextNumber :. }
function GetNextNumber(): Integer;
begin
Result := NextNumber;
Inc(NextNumber);
end;

{ TFooThread }

procedure TFooThread.Execute;
var
I: Integer;
begin
OnTerminate := Form1.MyThreadTerminate;

EnterCriticalSection(CS); // the beginning of a critical section
for I := 1 to ArrSize do
begin
GlobalArray[i] := GetNextNumber();
Sleep(3 + Random(12)); // let other threads work
end;
LeaveCriticalSection(CS); // the end of a critical section
end;

{ TForm1 }

procedure TForm1.Button1Click(Sender: TObject);
begin
InitializeCriticalSection(CS); // initializes critical section
TFooThread.Create(False);
TFooThread.Create(False);
end;

procedure TForm1.MyThreadTerminate(Sender: TObject);
var
I: Integer;
begin
Inc(DoneFlags);
if (DoneFlags = 2) then
begin
for I := 1 to ArrSize do
ListBox1.Items.Add(IntToStr(GlobalArray[I]));
DeleteCriticalSection(CS); // deletes critical section
end;
end;

end.
[/pascal]

Sorry for such a long snippet, but I thought it could be useful for you. What is the most important in this code? This little fragment:

[pascal]
for I := 1 to ArrSize do
begin
GlobalArray[I] := GetNextNumber();
Sleep(3 + Random(12)); // let other threads work
end;
[/pascal]

Don't forget to use Sleep command, because you allow other threads work then. It's very important!

Hope that helps.

[Diaboli]

thx!

The criticalsections i use are taken from a guide to multithreading that AthenaOfDelphi has written. i'll see if i can rewrite it to use your method instead, if it is needed.

My topmost priority, however, is to repair my XXXToRaw functions, wich are not working....

IntToRaw
SingleToRaw
++

and i forgot the sleep

as a sidenote:
Vista is supposed to be viritually "crash proof" is the sense that even if the UI crashes, it can restart it without restarting the OS itself.

now: what if you make a bunch of threads with the priority "Time Critical", and they all contain unending loops. would they steal prosessing cycles even from the OS?

[Brainer]

Maybe this will help you a bit:
[pascal]
{

RAW Data Formats
Based on Indy 19 structures.

}
program RAW;

{$APPTYPE CONSOLE}

uses
Math;

type
{ .: TRAWData :. }
TRAWData = array of Byte;

{ .: StringToRAW :. }
function StringToRAW(const Value: String): TRAWData;
begin
SetLength(Result, Length(Value));
if (Value <then> 0) then
Move(Value[Start], Result[1], Count);
end;

{ .: RAWToInt :. }
function RAWToInt(const Value: TRAWData): Integer;
begin
Result := PInteger(@Value[0])^;
end;

{ .: RAWToBoolean :. }
function RAWToBoolean(const Value: TRAWData): Boolean;
begin
Result := PBoolean(@Value[0])^;
end;

{ .: RAWToFloat :. }
function RAWToFloat(const Value: TRAWData): Single;
begin
Result := PSingle(@Value[0])^;
end;

var
S: String;
I: Integer;
B: Boolean;
F: Single;
R: TRAWData;
begin
{ ** VARIABLES INITIALIZATION ** }
S := 'Hito wa mikake ni yoranu mono' + #0;
I := 15;
B := True;
F := 178.5;

{ ** STRING -> RAW | RAW -> STRING ** }
R := StringToRAW(S);
S := '';
S := RAWToString(R, 0, 100);
Writeln(S);

{ ** INTEGER -> RAW | RAW -> INTEGER ** }
R := IntToRAW(I);
I := 0;
I := RAWToInt(R);
Writeln(I);

{ ** BOOLEAN -> RAW | RAW -> BOOLEAN ** }
R := BooleanToRAW(B);
B := False;
B := RAWToBoolean(R);
Writeln(B);

{ ** SINGLE -> RAW | RAW -> SINGLE ** }
R := FloatToRAW(F);
F := 0.0;
F := RAWToFloat(R);
Writeln(F:0:2);

Readln;
end.
[/pascal]

now: what if you make a bunch of threads with the priority "Time Critical", and they all contain unending loops. would they steal prosessing cycles even from the OS?

I guess it'll give you a system crash.