Indy10 - TIdTCPConnection, Open/Close, ReadLnTimedOut, Read Thread

[Michael Stieler]

Hello,

a write an application that has to handle multiple TCP Connections.
Two of them have sort of a telnet protocol, the others are binary.
For each connection I create an "Object" inherited from TComponent, in
which the connection is initialized as follows:

conn := TIdTcpConnection.Create(self);
conn.CreateIOHandler;
conn.Socket.WriteBufferOpen;
conn.Socket.OnStatus := TcpStatus;

The telnet sort connection is a Command+Response system. Because of
problems I had with the IdTelnetClient, I also use the IdTcpConnection
class here (Linefeed is LF+CR for one response, the others CR+LF)

Executing a command works like that:
conn.Socket.Write(strCommand+CR+LF);
A := conn.Socket.ReadLn(CR+LF);

1. Question
-----------
This works for 99% of the commands, and then I get a Timeout in the
ReadLn. And I even get it when I set the ReadTimeout up to 2500ms,
comparing to a TCP Terminal program this can't be the Server's fault,
there the response is always at once received. It seems to get worse
when talking to two Servers, although I call ReadLn sequentially. I put
the periodically execution of the commands in a TThread so the main
application doesn't freeze, but what could it be that I get those
ReadLnTimeOuts so often?

2. Question
-----------
Is my method of connecting/disconnecting right?

Connecting
If (conn.Socket.Opened) then conn.Socket.Close;
Setting Host/Port
conn.Socket.Open;

Writing
conn.Socket.Write(String);
conn.Socket.WriteBufferFlush;
(The protocol is very time critical, I assumed this returns faster than
using no write buffer)

Disconnecting
conn.Socket.Close;
conn.Socket.InputBuffer.Clear;
(Data left in InputBuffer could make the connection look like open)

I experimented with Disconnect; and Connect; and so on but the
Open/Close methods of the Socket seemed to be most direct to me.


3. Question
-----------
For the binary data connections I created a TReadThread =
Class(TThread), that is created when connection is opened, and
terminated/freed when connection is closed.

Sometimes it occurs, that the thread doesn't terminate. I close the
connection as follows:

conn.Socket.Close;
conn.Socket.InputBuffer.Clear;
if readThread <> nil then begin
// Between here..
readThread.Terminate;
readThread.WaitFor;
FreeAndNil(readThread);
// and here the program freezes in CsrNewThread of a System DLL
end;

The thread is as easy as this (without exception handling):

while (Not Terminated) and conn.Socket.Opened do begin

With conn.Socket do begin

CheckForDataOnSource(1000);
If not Opened then break;

I := InputBuffer.Size;
If I>0 then begin

S := ReadString(I);
If Assigned( FOnRead ) then
FonRead(conn, S); // Not Synchronized read event

end;
end;
End;

One time it happened, the tracer showed me the program was in
CheckForDataSource() method. Is it possible that this methode doesn't
return? I read about this method is not thread safe. I don't access
buffer or read functions during the run-time of this thread.


Indy-Version is the Dev-Snapshot from Fulgan.com

Thanks for your help..

Michael Stieler

[Michael Stieler]

Post by Michael Stieler
conn.Socket.Write(strCommand+CR+LF);
A := conn.Socket.ReadLn(CR+LF);

It seems that I have finally answered one of my questions:
conn.Socket.Write(command+CR+LF);
conn.Socket.CheckForDataOnSource(1000);
Result := conn.Socket.ReadLn(CR+LF);

This works, so apparently the ReadLn-Procedure doesn't check if data can
be received but perhaps just works from the InputBuffer.
CheckForDataOnSource() calls the protected methods to receive more data
into the buffer.

I hope you'll find solutions to my other problems as well.

M. Stieler

[Remy Lebeau (TeamB)]

Not really.

Post by Michael Stieler
conn.Socket.CheckForDataOnSource(1000);

You do not need to do that. ReadLn() handles that internally for you,
as do all of the other Read...() methods.

Post by Michael Stieler
This works, so apparently the ReadLn-Procedure doesn't check
if data can be received

Yes, it does.

Post by Michael Stieler
perhaps just works from the InputBuffer.

ReadBytes() gets its data from the InputBuffer, and all of the other
Read...() methods are implemented to use ReadBytes(). When the
InputBuffer does not hold enough bytes to satisfy a read operation,
ReadBytes() calls ReadFromSource() to poll the socket and put any
pending data into the InputBuffer.


Gambit

[Remy Lebeau (TeamB)]

Post by Michael Stieler
This works for 99% of the commands

I doubt that. You are using write buffering with no threshold
specified. Which means that no data will ever be written to the
socket until either WriteBufferFlush() or WriteBufferClose() are
called. If you forget to call it, the data is not sent to the server.
Worse, if something else calls WriteBufferOpen() internally before you
can flush it, then your data is completely lost.

Post by Michael Stieler
I get a Timeout in the ReadLn.

Because you are using write buffering, it is likely that not all of
your command data is actually being sent to the server, so the
ReadLn() times out because the server is still waiting for you to
finish sending a full command and thus is not sending back a response
at all.

Get rid of the write buffering. You are not using it correctly
anyway, and it is not gaining you anything useful.

Post by Michael Stieler
And I even get it when I set the ReadTimeout up to 2500ms,
comparing to a TCP Terminal program this can't be the Server's
fault, there the response is always at once received.

Terminal does not use write buffering, so there is no delay in data
being sent to the server, and thus no delay in its response.

Post by Michael Stieler
Is my method of connecting/disconnecting right?

No. You should not be creating a TIdTCPConnection directly in the
first place. You should be using a TIdTCPClient instead, ie:

conn := TIdTCPClient.Create(self);
conn.OnStatus := TcpStatus;
...
if conn.Connected then conn.Disconnect;
Set Host/Port
conn.Connect;
...
conn.IOHandler.Write(String);
...
conn.Disconnect;
conn.IOHandler.InputBuffer.Clear;

Post by Michael Stieler
The protocol is very time critical, I assumed this returns faster
than using no write buffer

You assume wrong. Write buffering is only useful when you perform
multiple writes between the WriteBufferOpen() and
WriteBufferClose/Flush() calls, or sending very large data blocks.
But you are not doing that. You are flushing the buffer after each
Write(). Which effectively circumvents what write buffering is all
about.

Post by Michael Stieler
I experimented with Disconnect; and Connect; and so on but
the Open/Close methods of the Socket seemed to be most
direct to me.

You are not supposed to be using them directly.

Post by Michael Stieler
For the binary data connections I created a TReadThread =
Class(TThread), that is created when connection is opened, and
terminated/freed when connection is closed.

That is ok to do.

Post by Michael Stieler
Sometimes it occurs, that the thread doesn't terminate.

That is because you are not disconnect the socket properly, so the
thread does not detect it so it can abort any operations that are
pending.
Make sure that your exception handler is allowing the thread to
terminate.

Post by Michael Stieler
while (Not Terminated) and conn.Socket.Opened do begin

while (not Terminated) and (conn.Connected) do begin

Post by Michael Stieler
I := InputBuffer.Size;
If I>0 then begin
S := ReadString(I);

Use InputBufferAsString() instead:

S := InputBufferAsString;
if S <> '' then

Post by Michael Stieler
One time it happened, the tracer showed me the program was in
CheckForDataSource() method. Is it possible that this methode
doesn't return?

Indy uses the socket API select() function to implement its timeouts.
In very very rare occasions, select() has been known to freeze up at
the OS level. Byt it is so rare as to not really even be a problem.

Post by Michael Stieler
I read about this method is not thread safe.

It is fine, as long as no other thread is trying to read from the same
socket.


Gambit

[Michael Stieler]

Post by Remy Lebeau (TeamB)
No. You should not be creating a TIdTCPConnection directly in the

Thank you, that is the way I first wanted to do it. Then ran into
problems and tried about every method of connecting and getting data. It
now works better but I got another problem back I hoped to have eliminated:

I now use a TIdTcpClient I create in a self-built TComponent class. This
class has a Connect routine that looks like this:

conn.Host := ...
conn.Port := ...
RIELog('Connect');
conn.Connect;

RIELog is a global Logging routine that adds a line to a Memo box.
In the OnConnected Event Handler there is

RIELog('Connected');
readThread := TReadThread.Create(conn);
readThread.OnRead := Read;
readThread.OnTerminate := ThreadTerminated;

After disconnection I terminate the thread with .Terminate and Free it
in the OnTerminate event handler.

Now my program connects, disconnects, connects, disconnects.. and on the
second or third or even fifth attempt, it deadlocks. The last Log entry
is Connected and debugger is in CPU mode with no call stack or links to
ntdll.RtlTryEnterCriticalSection. I don't understand why, and debug info
makes the .Connect method of the TcpClient suspicious.

The class methods run in main thread context, except the OnRead handler
which is triggered by the read thread. Another thread is holding another
TCP Connection with only Write and ReadLn commands.

Any tips?

Michael Stieler

[Michael Stieler]

Post by Remy Lebeau (TeamB)
conn.Connect;

Again a small addition... if I .Free and .Create the TcpClient every
time before calling .Connect there seems to be no dead-lock. Maybe I do
something wrong on disconnect? The error never occurs on the first connect.

I use conn.Disconnect and in the OnDisconnected event handler:

if Assigned(readThread) then
readThread.Terminate; // FreeOnTerminate := true

If Assigned(conn.IOHandler) then
conn.IOHandler.InputBuffer.Clear;

// Notify application (fire event)
If Assigned(FOnDisconnect) then
FOnDisconnect(self);


I don't use WriteBuffering.

Thanks,
Michael Stieler

[Michael Stieler]

Post by Michael Stieler

Post by Remy Lebeau (TeamB)
conn.Connect;

Again a small addition... if I .Free and .Create the TcpClient every
time before calling .Connect there seems to be no dead-lock. Maybe I do
something wrong on disconnect? The error never occurs on the first connect.

Sorry for spamming... but the dead-lock still occurs. Just took longer now.

Regards,
Michael

[Remy Lebeau (TeamB)]

Post by Michael Stieler
if Assigned(readThread) then
readThread.Terminate; // FreeOnTerminate := true

Don't use FreeOnTerminate in this situation.


Gambit

[Remy Lebeau (TeamB)]

It now works better but I got another problem back I hoped to have

eliminated:

Which is what exactly?

After disconnection I terminate the thread with .Terminate

Are you calling WaitFor(), though? You should be.

and Free it in the OnTerminate event handler.

Do not do that! That is not a valid place to free the thread. That
is a good way to crash the VCL and deadlock other threads. Just free
the thread after WaitFor() has exited instead, ie:

constructor TMyComp.Create(AOwner: TComponent);
begin
conn := TIdTCPClient.Create(Self);
end;

procedure TMyComp.Connect;
begin
conn.Host := ...
conn.Port := ...
RIELog('Connecting');
conn.Connect;
try
readThread := TReadThread.Create(conn);
readThread.OnRead := Read;
readThread.Resume;
except
conn.Disconnect;
raise;
end;
RIELog('Connected');
end;

procedure TMyComp.Disconnect;
begin
RIELog('Disconnecting');
if readThread <> nil then readThread.Terminate;
try
conn.Disconnect;
finally
if readThread <> nil then
begin
readThread.WaitFor;
FreeAndNil(readThread);
end;
end;
RIELog('Disconnected');
end;


Gambit

[Michael Stieler]

Post by Remy Lebeau (TeamB)

Post by Michael Stieler
After disconnection I terminate the thread with .Terminate

Are you calling WaitFor(), though? You should be.

Post by Michael Stieler
and Free it in the OnTerminate event handler.

Do not do that! That is not a valid place to free the thread. That
is a good way to crash the VCL and deadlock other threads. Just free

Okay, I described this wrong.. but the point I'll try to build in is the
order of commands.. 1. Terminate 2. Disconnect 3. WaitFor 4. Free

What if the read thread finishes because its own while condition not
met? Does it harm to call .Terminate on it after this when
.FreeOnTerminate = false? I once had the impression that .WaitFor didn't
return if the thread is already finished. But why shouldn't it..

Post by Remy Lebeau (TeamB)
constructor TMyComp.Create(AOwner: TComponent);
begin
conn := TIdTCPClient.Create(Self);
end;
procedure TMyComp.Connect;
begin
conn.Host := ...
conn.Port := ...
RIELog('Connecting');
conn.Connect;
try
readThread := TReadThread.Create(conn);
readThread.OnRead := Read;
readThread.Resume;
except
conn.Disconnect;
raise;
end;
RIELog('Connected');
end;

You wanted to put the conn.Connect inside the try..except block, didn't
you? Then I'd understand it.

Post by Remy Lebeau (TeamB)
Gambit

Thank you, I'll try to fix it on Monday and report how it worked.

Michael

[Remy Lebeau (TeamB)]

Post by Michael Stieler
What if the read thread finishes because its own while
condition not met?

It does not matter why the thread exits from its Execute() method.
Once Execute() has exited, the thread is effectly finished and
WaitFor() can then return immediately afterwards.

Post by Michael Stieler
Does it harm to call .Terminate on it after this when

.FreeOnTerminate = false?

All the Terminate() method does it sets the Terminated property to
true. It does not do anything else.

Post by Michael Stieler
I once had the impression that .WaitFor didn't return if the
thread is already finished.

Yes, it does.

Post by Michael Stieler
You wanted to put the conn.Connect inside the try..except block,
didn't you?

No, I did not. I intentionally left it out. If Connect() fails, an
exception is raised. There is no point in creating the thread when
that happens, and the code snippet I showed you is not providing any
other way of reporting whether Connect() failed, so I am letting the
exception escape into the calling code for the application to handle
as it sees fit. The only reason to put Connect() inside the
try..except block would be if the Connect() method had a Boolean
return value instead, ie:

function TMyComp.Connect: Boolean;
begin
Result := False;
conn.Host := ...
conn.Port := ...
try
conn.Connect;
try
readThread := TReadThread.Create(conn);
readThread.OnRead := Read;
readThread.Resume;
except
conn.Disconnect;
raise;
end;
Result := True;
except
end;
end;


Gambit

[Michael Stieler]

Post by Remy Lebeau (TeamB)

Post by Michael Stieler
What if the read thread finishes because its own while
condition not met?

It does not matter why the thread exits from its Execute() method.
Once Execute() has exited, the thread is effectly finished and
WaitFor() can then return immediately afterwards.

It still doesn't work as expected.

My read thread Execute procedure:

procedure TReadThread.Execute;
var
S : String;
Begin
try

while (Not Terminated) and conn.Connected do begin
try

conn.IOHandler.CheckForDataOnSource(600);
S := conn.IOHandler.InputBufferAsString;
If S <> '' then
If Assigned( FOnRead ) then
FonRead(conn, S);

except
// Something went wrong, exit loop
break;
end;

End;
except
// catch thread exceptions to avoid access violations
end;
end;


And it fails in the Disconnect procedure:

procedure TModuleProxy.Disconnect;
begin

RIELog('(MP) Before Thread Termination');

if readThread<>nil then begin
readThread.Terminate;
readThread.WaitFor;
FreeAndNil(readThread);
end;

RIELog('After Thread Termination');
// this log is never done and readThread is not nil!
try
conn.Disconnect;
except
// ignore
end;

end;


Any ideas why this could still fail? On a new .Connect() readThread is
not nil and sometimes the program freezes.

Michael

[Michael Stieler]

Post by Michael Stieler
It still doesn't work as expected.
...
Any ideas why this could still fail? On a new .Connect() readThread is
not nil and sometimes the program freezes.
Michael

Oops.. FreeOnTerminate was still true, with false this part works as
expected. Thanks!

Michael