Recent

Author Topic: Indy TIdTCPServer on disconnection  (Read 4293 times)

torbente

  • Sr. Member
  • ****
  • Posts: 311
    • Noso Main Page
Indy TIdTCPServer on disconnection
« on: May 20, 2021, 08:24:44 am »
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][+][-]
  1. Acontext.Connection.IOHandler.InputBuffer.Clear;
  2.  

When a client is disconnected.
Are we missing something?

Thanks in advance.
Noso Cryptocurrency Main Developer
https://github.com/DevTeamNoso/NosoWallet

Remy Lebeau

  • Hero Member
  • *****
  • Posts: 952
    • Lebeau Software
Re: Indy TIdTCPServer on disconnection
« Reply #1 on: May 20, 2021, 06:04:48 pm »
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?

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.

On all cases we added

Code: Pascal  [Select][+][-]
  1. Acontext.Connection.IOHandler.InputBuffer.Clear;
  2.  

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.

Are 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.
« Last Edit: May 25, 2021, 08:57:05 pm by Remy Lebeau »
Remy Lebeau
Lebeau Software - Owner, Developer
Internet Direct (Indy) - Admin, Developer (Support forum)

torbente

  • Sr. Member
  • ****
  • Posts: 311
    • Noso Main Page
Re: Indy TIdTCPServer on disconnection
« Reply #2 on: May 25, 2021, 05:30:24 pm »
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][+][-]
  1. conectiondata = Packed Record  
  2.   // integer, string and boolean types
  3.   context: TIdContext;
  4.   end;
  5. MyConnections: array [1..MaxConnections] of conectiondata; // fixed size
  6.  

This way, when a new connection enters:

Code: Pascal  [Select][+][-]
  1. procedure TForm1.IdTCPServer1Connect(AContext: TIdContext);  
  2. var
  3.   Lline : string;
  4. Begin
  5. LLine := AContext.Connection.IOHandler.ReadLn('',200,-1,IndyTextEncoding_UTF8);  
  6. if AContext.Connection.IOHandler.ReadLnTimedout then
  7.    begin
  8.    AContext.Connection.Disconnect;  
  9.    exit;
  10.    end;
  11. // parse the line, if handshake is good...
  12. addnewconnection(AContext); // and leaves the connection open
  13. // if not good handshake
  14. AContext.Connection.Disconnect;
  15. End;
  16.  
  17. procedure addnewconnection(concontext:TIdContext);
  18. var
  19.   newdata : conectiondata;
  20. Begin
  21. // mainly do this...
  22. newdata := default(conectiondata);
  23. newdata.context := concontext:TIdContext;
  24. // and inserts the data in a free slot on the MyConnections array;
  25. MyConnections[availableslot] := newdata;
  26. End;

And whenever a disconnection event happens (we close it, the client disconnected or an exception happened):

Code: Pascal  [Select][+][-]
  1. Procedure ClearConnectionSlot(concontext:TIdContext);
  2. Begin
  3. // search in the array MyConnections for the record with same concontext and
  4. MyConnections[slottobecleared].context.Connection.Disconnect;  
  5. MyConnections[slottobecleared] := Default(conectiondata);
  6. 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.
Noso Cryptocurrency Main Developer
https://github.com/DevTeamNoso/NosoWallet

Remy Lebeau

  • Hero Member
  • *****
  • Posts: 952
    • Lebeau Software
Re: Indy TIdTCPServer on disconnection
« Reply #3 on: May 25, 2021, 09:07:34 pm »
We 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.

This 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.

We 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.
Remy Lebeau
Lebeau Software - Owner, Developer
Internet Direct (Indy) - Admin, Developer (Support forum)

torbente

  • Sr. Member
  • ****
  • Posts: 311
    • Noso Main Page
Re: Indy TIdTCPServer on disconnection
« Reply #4 on: May 25, 2021, 10:12:00 pm »
Thanks again Remy.

Quote
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.

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.

Quote
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.

OnConnect is completely inside a critical section:

Code: Pascal  [Select][+][-]
  1. procedure TForm1.IdTCPServer1Connect(AContext: TIdContext);  
  2. Begin
  3. EnterCriticalSection(CSPeerJoin);
  4. ...
  5. LeaveCriticalSection(CSPeerJoin);
  6. End;

And OnExecute, onDisconnect and onException uses CriticalSections to access MyConnections array and other data structures.

Quote
I 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][+][-]
  1. // returns the number of active connections
  2. function TForm1.ServerClientsCount : Integer ;
  3. var
  4.   Clients : TList;
  5. begin
  6.   Clients:= IdTCPServer1.Contexts.LockList;
  7.   try
  8.     Result := Clients.Count ;
  9.   finally
  10.     IdTCPServer1.Contexts.UnlockList;
  11.   end;
  12. 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][+][-]
  1. procedure TForm1.IdTCPServer1Connect(AContext: TIdContext);  
  2. var
  3.    ConnectionAdded :boolean = false;
  4. Begin
  5. EnterCriticalSection(CSPeerJoin);
  6. ...
  7. if handshakeOK then
  8.    begin
  9.    addnewconnection(AContext);
  10.    ConnectionAdded ;= true;
  11.    end;
  12. ...
  13. if not ConnectionAdded then AContext.Connection.Disconnect;
  14. LeaveCriticalSection(CSPeerJoin);
  15. 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][+][-]
  1. if length(MyConnections) <> ServerClientsCount then
  2.    begin
  3.    for counter := 0 to ServerClientsCount-1 do
  4.       if IdTCPServer1.Contexts[counter] doNotExistsInMyConnections then
  5.          IdTCPServer1.Contexts[counter].Connection.Disconnect;
  6.    end;

Thanks a lot. We have spent many time with this issue.
Noso Cryptocurrency Main Developer
https://github.com/DevTeamNoso/NosoWallet

Remy Lebeau

  • Hero Member
  • *****
  • Posts: 952
    • Lebeau Software
Re: Indy TIdTCPServer on disconnection
« Reply #5 on: May 25, 2021, 11:28:17 pm »
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.

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.

It 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.

Could 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.

With 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).

and 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.

Code: Pascal  [Select][+][-]
  1. 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?

Code: Pascal  [Select][+][-]
  1. begin
  2.    for counter := 0 to ServerClientsCount-1 do
  3.       if IdTCPServer1.Contexts[counter] doNotExistsInMyConnections then
  4.          IdTCPServer1.Contexts[counter].Connection.Disconnect;
  5.    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().
Remy Lebeau
Lebeau Software - Owner, Developer
Internet Direct (Indy) - Admin, Developer (Support forum)

torbente

  • Sr. Member
  • ****
  • Posts: 311
    • Noso Main Page
Re: Indy TIdTCPServer on disconnection
« Reply #6 on: May 26, 2021, 04:07:06 am »
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][+][-]
  1. procedure TForm1.IdTCPServer1Connect(AContext: TIdContext);  
  2. Begin
  3. Serverindex := //Acontext.Serverindex?
  4. 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][+][-]
  1.  IdTCPServer1.Contexts[Serverindex].Connection.IOHandler.WriteLn(//whatever)

Quote
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().

We will implemented this when the code is closing the connection from server side. Did not even know this neither.
Noso Cryptocurrency Main Developer
https://github.com/DevTeamNoso/NosoWallet

Remy Lebeau

  • Hero Member
  • *****
  • Posts: 952
    • Lebeau Software
Re: Indy TIdTCPServer on disconnection
« Reply #7 on: May 26, 2021, 08:21:07 pm »
How 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.

So 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.

Code: Pascal  [Select][+][-]
  1. IdTCPServer1.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.
« Last Edit: May 26, 2021, 08:23:49 pm by Remy Lebeau »
Remy Lebeau
Lebeau Software - Owner, Developer
Internet Direct (Indy) - Admin, Developer (Support forum)

 

TinyPortal © 2005-2018