Indy TIdTCPServer on disconnection
lazarus / 38-Networking and Web Programming

----------------------------------------------------------------------
torbente · 2021-05-20 06:24

Hi
Using Indy 10 on win 8.1 / Lazarus 2.0.10

We are using a TIdTCPServer to receive connections from clients. We
noticed that after some time, the memory usage of the app increses
slowly. We check all the code and we do not found memory leeks. Maybe
we need free memory when a client disconnects? On all cases we added

Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i
Acontext.Connection.IOHandler.InputBuffer.Clear; 
When a client is disconnected.
Are we missing something?

Thanks in advance.

----------------------------------------------------------------------
Remy Lebeau · 2021-05-20 16:04

Quote from: torbente on May 20, 2021, 08:24:44 amWe noticed that after
some time, the memory usage of the app increses slowly. We check all
the code and we do not found memory leeks. Maybe we need free memory
when a client disconnects?

Only memory you have allocated yourself.  Such as if you are
allocating memory to store in the TIdContext.Data property.  Or if you
have derived a new class from TIdServerContext and added additional
data members to it.  Anything else related to Indy connections is
owned by Indy and will be managed accordingly.

Quote from: torbente on May 20, 2021, 08:24:44 amOn all cases we added

Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i
Acontext.Connection.IOHandler.InputBuffer.Clear; 
When a client is disconnected.

That step is not necessary.  After the OnDisconnect event handler
exits, the Context will be freed, which will also free the Connection,
its IOHandler, and its InputBuffer.

Quote from: torbente on May 20, 2021, 08:24:44 amAre we missing
something?

Yes.  The question is - where?  If there is no actual leak, maybe you
are just fragmenting memory instead?  Hard to say without a
reproducible example.

----------------------------------------------------------------------
torbente · 2021-05-25 15:30

Thanks remy for your answer.

This is the same code we use but without try..except to make it easier
to understand.
We have an array of connectiondata, that stores the TidContext:

Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i
conectiondata = Packed Record    // integer, string and boolean types 
context: TIdContext;  end;MyConnections: array [1..MaxConnections] of
conectiondata; // fixed size 
This way, when a new connection enters:

Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i procedure
TForm1.IdTCPServer1Connect(AContext: TIdContext);   var  Lline :
string;BeginLLine :=
AContext.Connection.IOHandler.ReadLn('',200,-1,IndyTextEncoding_UTF8);
 if AContext.Connection.IOHandler.ReadLnTimedout then    begin 
 AContext.Connection.Disconnect;     exit;   end;// parse the line, if
handshake is good...addnewconnection(AContext); // and leaves the
connection open// if not good handshakeAContext.Connection.Disconnect;
End; procedure addnewconnection(concontext:TIdContext);var  newdata :
conectiondata;Begin// mainly do this...newdata :=
default(conectiondata);newdata.context := concontext:TIdContext;// and
inserts the data in a free slot on the MyConnections
array;MyConnections[availableslot] := newdata;End;
And whenever a disconnection event happens (we close it, the client
disconnected or an exception happened):

Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i Procedure
ClearConnectionSlot(concontext:TIdContext);Begin// search in the array
MyConnections for the record with same concontext
andMyConnections[slottobecleared].context.Connection.Disconnect;
 MyConnections[slottobecleared] := Default(conectiondata);End;
We wonder if there is a memory leak here; as far as we understand,
clearing the array record with := default should keep it clean. We
look at indy tcpsustomserver and we do not see any reason to believe
something else.

----------------------------------------------------------------------
Remy Lebeau · 2021-05-25 19:07

Quote from: torbente on May 25, 2021, 05:30:24 pmWe have an array of
connectiondata, that stores the TidContext:

Why?  TIdTCPServer has its own Contexts property for tracking
connections, and you can attach your own data to the TIdContext.Data
property, or derive a custom class from TIdServerContext and set the
server's ContextClass property.

Quote from: torbente on May 25, 2021, 05:30:24 pmThis way, when a new
connection enters:
...
And whenever a disconnection event happens (we close it, the client
disconnected or an exception happened):
...

Is that code thread-safe?  Remember that TIdTCPServer is a multi-
threaded component. Each TIdContext is run in its own thread, and the
OnConnect/OnExecute/OnDisconnect events are called in those threads. 
So make sure you are serializing access to your array properly.

Quote from: torbente on May 25, 2021, 05:30:24 pmWe wonder if there is
a memory leak here;

I see nothing in the code shown that can leak, since there is nothing
being allocated, so nothing to free.

----------------------------------------------------------------------
torbente · 2021-05-25 20:12

Thanks again Remy.

QuoteWhy?  TIdTCPServer has its own Contexts property for tracking
connections, and you can attach your own data to the TIdContext.Data
property, or derive a custom class from TIdServerContext and set the
server's ContextClass property.
We did it this way ~3 years ago before we know that. Yes, it is doing
the same job TIdTCPServer already do, but it was easier to keep it
once our job was done.

QuoteIs that code thread-safe?  Remember that TIdTCPServer is a multi-
threaded component. Each TIdContext is run in its own thread, and the
OnConnect/OnExecute/OnDisconnect events are called in those threads. 
So make sure you are serializing access to your array properly.
OnConnect is completely inside a critical section:

Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i procedure
TForm1.IdTCPServer1Connect(AContext: TIdContext);  BeginEnterCriticalS
ection(CSPeerJoin);...LeaveCriticalSection(CSPeerJoin);End;
And OnExecute, onDisconnect and onException uses CriticalSections to
access MyConnections array and other data structures.

QuoteI see nothing in the code shown that can leak, since there is
nothing being allocated, so nothing to free.
For debug we implemented this function some days ago to know how many
connections is handling the server: (we got it from another thread in
this forums)

Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i // returns
the number of active connectionsfunction TForm1.ServerClientsCount :
Integer ;var  Clients : TList;begin  Clients:=
IdTCPServer1.Contexts.LockList;  try    Result := Clients.Count ; 
finally    IdTCPServer1.Contexts.UnlockList;  end;end ;
And when we print: (Myconnections array fixed size is 1..200)
length(MyConnections)/ServerClientsCount
we got 200/200... for some time...then slowly the second number slowly
increases (average ~1 per hour), so it means that there are some
connections/threads in the server that "we are not handling" properly.
It is pretty weird because the onConnect procedure have a call to
disconnect the new connection if it was not added to myconnections:

Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i procedure
TForm1.IdTCPServer1Connect(AContext: TIdContext);  var 
 ConnectionAdded :boolean =
false;BeginEnterCriticalSection(CSPeerJoin);...if handshakeOK then 
 begin   addnewconnection(AContext);   ConnectionAdded ;= true; 
 end;...if not ConnectionAdded then
AContext.Connection.Disconnect;LeaveCriticalSection(CSPeerJoin);End;
There is something in our code that somehow allows un-tracked
connections anyway! And of course, those connections are flooding the
memory slowly...
Could you give us an idea how we could access the Server contexts
lists? With it, we could compare with the Myconnection contexts and
get what are the not tracked one to close those connections. (and in
the way, determine where is the leak in our code that allows untracked
connections)

Pseudo code we want implement (how to check all server contexts?):

Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i if
length(MyConnections) <> ServerClientsCount then   begin   for counter
:= 0 to ServerClientsCount-1 do      if IdTCPServer1.Contexts[counter]
doNotExistsInMyConnections then       
 IdTCPServer1.Contexts[counter].Connection.Disconnect;    end;
Thanks a lot. We have spent many time with this issue.

----------------------------------------------------------------------
Remy Lebeau · 2021-05-25 21:28

Quote from: torbente on May 25, 2021, 10:12:00 pmAnd when we print:
(Myconnections array fixed size is 1..200)
length(MyConnections)/ServerClientsCount
we got 200/200... for some time...then slowly the second number slowly
increases (average ~1 per hour), so it means that there are some
connections/threads in the server that "we are not handling" properly.

Most likely, you are blocking the server from processing the
disconnects so it can clean up the contexts properly.  For instance,
if you were swallowing Indy exceptions and not letting the server
process them.  Can't be sure, since you have not shown enough of your
server code to know what it is actually doing, particularly is
OnExecute logic.

Quote from: torbente on May 25, 2021, 10:12:00 pmIt is pretty weird
because the onConnect procedure have a call to disconnect the new
connection if it was not added to myconnections:

That logic looks fine, provided it is adequately protected with
try..finally, and you are not going out of bounds of your
myconnections array.

Quote from: torbente on May 25, 2021, 10:12:00 pmCould you give us an
idea how we could access the Server contexts lists?

You already know how, since you are accessing it in
ServerClientsCount().  LockList() returns a pointer to a TList which
holds TIdContext object pointers.  You are merely reading the Count of
that list, but you can also iterate through it, too.

Quote from: torbente on May 25, 2021, 10:12:00 pmWith it, we could
compare with the Myconnection contexts

I doubt that will help.  Since you are adding contexts to your array
in the OnConnect event, and removing contexts from your array in the
OnDisconnect event, you are likely to find that the two lists are
always in sync containing the same TIdContext objects (provided you
are managing your array correctly).

Quote from: torbente on May 25, 2021, 10:12:00 pmand get what are the
not tracked one to close those connections.

If you find a TIdContext in your array that the server is not tracking
in its Contexts list, then that object is gone, you can't close it, or
do anything else with it.  You have a dangling pointer.

If you find a TIdContext in the server's Contexts list that your array
is not tracking, then your tracking logic is faulty.  Except in the
very small window of opportunity when a client connects/disconnects
where:

- a new client has been added to the Contexts list but has not been
added to your array yet.

- an existing client has been removed from your array but has not been
removed from the Contexts list yet.

This is another reason why I do not recommend using your own array at
all.  You are basically just mirroring the Contexts list redundantly.

Quote from: torbente on May 25, 2021, 10:12:00 pmCode: Pascal
 [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i if
length(MyConnections) <> ServerClientsCount then
You do realize that condition will always be true except when your
server is running at max capacity (200 clients), don't you?

Quote from: torbente on May 25, 2021, 10:12:00 pmCode: Pascal
 [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i begin   for
counter := 0 to ServerClientsCount-1 do      if
IdTCPServer1.Contexts[counter] doNotExistsInMyConnections then       
 IdTCPServer1.Contexts[counter].Connection.Disconnect;    end;
Make sure this loop is inside a LockList/UnlockList pair, or else the
ServerClientsCount can change value if clients connect/disconnect
while you are looping.

Also, that scan is only checking for TIdContext objects that exist in
the Contexts list but not in the array.  It is not checking for
TIdContext objects that exist in the array but not in the Contexts
list.

Also, when disconnecting a client from outside of its own events, it
may be safer to use
TIdContext(IdTCPServer1.Contexts[counter]).Binding.CloseSocket()
instead of
TIdContext(IdTCPServer1.Contexts[counter]).Connection.Disconnect().

----------------------------------------------------------------------
torbente · 2021-05-26 02:07

We discussed about this and seems like the best solution is to use the
already built-in TList, even if it means a lot of code work change.

How we can know the index the new connection have in the TList?

Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i procedure
TForm1.IdTCPServer1Connect(AContext: TIdContext);  BeginServerindex :=
//Acontext.Serverindex?end;
So we can store that number instead of the context, and when required
to send something to that connection use it, something like...

Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i  IdTCPServe
r1.Contexts[Serverindex].Connection.IOHandler.WriteLn(//whatever)
QuoteAlso, when disconnecting a client from outside of its own events,
it may be safer to use
TIdContext(IdTCPServer1.Contexts[counter]).Binding.CloseSocket()
instead of
TIdContext(IdTCPServer1.Contexts[counter]).Connection.Disconnect().
We will implemented this when the code is closing the connection from
server side. Did not even know this neither.

----------------------------------------------------------------------
Remy Lebeau · 2021-05-26 18:21

Quote from: torbente on May 26, 2021, 04:07:06 amHow we can know the
index the new connection have in the TList?

TList has an IndexOf() method.  But I would not suggest relying on
indexes at all, since the contents of the TList will change over time
as clients connect and disconnect, thus invalidating any stored
indexes.

Quote from: torbente on May 26, 2021, 04:07:06 amSo we can store that
number instead of the context, and when required to send something to
that connection use it

Better to store pointers to the actual TIdContext objects. Just make
sure you don't access them beyond their OnDisconnect events, since
they won't exist in memory anymore by that time.

Or, simply lookup the TIdContext in the TList dynamically on an as-
needed basis.  Lock the TList, search for the desired TIdContext as
needed, use it if found, and then unlock the TList.

Quote from: torbente on May 26, 2021, 04:07:06 amCode: Pascal
 [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i IdTCPServer
1.Contexts[Serverindex].Connection.IOHandler.WriteLn(//whatever)
Aside from not being able to track Serverindex reliably, another
problem with this approach is that it can actually be very dangerous.

It is perfectly safe to have one thread reading data from a socket at
the time same that another thread is writing data to the same socket,
without any coordination between the two threads.  However, you are
sending data from outside of the TIdContext's owning thread, so if the
OnExecute event also wants to send data to the same TIdContext, you
will need to coordinate between the two sending threads to ensure
integrity of your communications.

Also, because Indy uses blocking sockets, you typically don't want to
block your other threads if a client's socket needs to block because
its peer is slow/unresponsive.

Best to limit all of the actual socket communication with a given
TIdContext to just its own events only.

This is why I usually recommend in this kind of situation to give each
connected client its own thread-safe outbound queue that other threads
can push data into when needed, and then the OnExecute event can flush
that queue to the connection when it is safe to do so.

Whether or not you will need to do this depends on what your OnExecute
event actually looks like.

----------------------------------------------------------------------
torbente · 2022-01-24 02:11

Hi again; back after 8 months without been able to solve the mem
issue. 

Looking at this old post:
https://stackoverflow.com/questions/14103944/indy-10-tcp-server

We were able to figure out that OnExecute can be used also to send
messages to the clients, so we moved everything inside there, and now
we are peforming some tests to know if the problem is that the server
is sending data meanwhile there are incoming data in the buffer (or
socket is busy?) and corrupting all.

Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i
Form1.MyServer := TIdTCPServer.Create(Form1); SendMessage :
Boolean;GlobalText     : string;...procedure
TForm1.MyServerExecute(AContext: TIdContext);  Beginif SendMessage
then   begin   MyServer.Contexts.LockList;  
 Acontext.Connection.IOHandler.WriteLn(GlobalText); 
 MyServer.Contexts.UnlockList;    SendMessage  := false;   end;End;
Is this the correct way to safely send data to the client? The mem
leak is still there, even if we add a
AContext.Connection.IOHandler.InputBufferIsEmpty=true verification
before sending the line.

We have to admit that somehow we believed that IOHandler had the job
to organize the I/O properly to avoid this issues, but we discovered
the hard way that we were wrong...

----------------------------------------------------------------------
Remy Lebeau · 2022-01-24 18:45

Quote from: torbente on January 24, 2022, 03:11:43 amCode: Pascal
 [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i
Form1.MyServer := TIdTCPServer.Create(Form1); SendMessage :
Boolean;GlobalText     : string;...procedure
TForm1.MyServerExecute(AContext: TIdContext);  Beginif SendMessage
then   begin   MyServer.Contexts.LockList;  
 Acontext.Connection.IOHandler.WriteLn(GlobalText); 
 MyServer.Contexts.UnlockList;    SendMessage  := false;   end;End;
Why are you locking the Contexts list like that? You DO NOT need to do
that in this situation.  Look at my earlier example again more
carefully.  The locking of the list is only needed when accessing the
connections from OUTSIDE of their OnExecute events, not INSIDE of the
events, eg:

Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i
Form1.MyServer := TIdTCPServer.Create(Form1); procedure
TForm1.MyServerExecute(AContext: TIdContext);  Begin  if SendMessage
then  begin    AContext.Connection.IOHandler.WriteLn(GlobalText);   
SendMessage := false;  end;End;
Quote from: torbente on January 24, 2022, 03:11:43 amIs this the
correct way to safely send data to the client?

No, because your use of global variables is not thread-safe across
multiple connections.  Did you even try my earlier
TIdThreadSafeStringList example?  Or, if you only want 1 string queued
at a time, you could use TIdThreadSafeString instead.  Of course,
these are just examples, there are many ways to approach this.

Quote from: torbente on January 24, 2022, 03:11:43 amThe mem leak is
still there, even if we add a
AContext.Connection.IOHandler.InputBufferIsEmpty=true verification
before sending the line.

As I stated earlier, and will state again now - there are no memory
leaks in any of the code presented so far.  So your problem has to be
elsewhere in code you have not presented here.

Quote from: torbente on January 24, 2022, 03:11:43 amWe have to admit
that somehow we believed that IOHandler had the job to organize the
I/O properly to avoid this issues, but we discovered the hard way that
we were wrong...

The IOHandler is just a gateway to the underlying socket.  You are
still responsible for managing proper thread synchronization in your
own code.

----------------------------------------------------------------------
eldonfsr · 2022-03-02 14:22

Hi sorry i ask that how you define or determine what is the
slottobecleared , sorry just learning about indy and web programing on
lazarus...

I found some code sample but a sample to follow to handle server i
don't see any...

Thanks...

----------------------------------------------------------------------
Remy Lebeau · 2022-03-02 16:48

Quote from: eldonfsr on March 02, 2022, 03:22:36 pmHi sorry i ask that
how you define or determine what is the slottobecleared

In the earlier example, that would be handled by looping through the
MyConnections[] array looking for an entry whose Context member
matches the passed-in TIdContext parameter - just as the comment in
that example says:

QuoteCode: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i // search
in the array MyConnections for the record with same concontext