I have an Indy TCP server, the client connects to it, send an information, the server receives, create a huge TStringList and send back to the client. This happens thousands of times daily so I decided to put six servers on different .exe doing the same thing on different ports and put the client app to connect each time on a random port.
What is happening:
1) The moment from the client first attempt to connect to the moment he receive all the information he needs is pretty high, like he was doing the work twice.
2) Around 10% of the times the client tries to connect and the server seems to ignore the attempt and the client doesn't fire the except to try again, and stuck.
The server:
MaxConnections is set to 1ListenQueue to 0TerminateWaitTime to 5000ReuseSocket to False on client and serverUsaNagle := True on client and serverThe client 6 second Timer that tries to connect to the server:
IdDownloadClient.IOHandler.MaxLineLength := MaxInt;
IdDownloadClient.IOHandler.DefStringEncoding := IndyTextEncoding_UTF8;
// Choose a random port
IdDownloadClient.Connect;
IdDownloadClient.IOHandler.WriteLn(lat+','+long);
Timer6.Enabled := True;
The client Timer6 (25 ms):
with IdDownloadClient do
begin
try
if IOHandler.InputBufferIsEmpty then
begin
IOHandler.CheckForDataOnSource(0);
IOHandler.CheckForDisconnect;
if IOHandler.InputBufferIsEmpty then Exit;
end;
end;
receivedtext := IOHandler.ReadLn;
except
Timer6.Enabled := False;
Exit;
end;
if receivedtext = '@' then begin // The server send an '@' to say the complete TStringList has been sent
IdDownloadClient.IOHandler.InputBuffer.Clear;
IdDownloadClient.IOHandler.CloseGracefully;
IdDownloadClient.Disconnect;
The server OnExecute event:
begin
try
AContext.Connection.IOHandler.DefStringEncoding := IndyTextEncoding_UTF8;
LatLong := AContext.Connection.IOHandler.ReadLn;
if LatLong <> '' then begin
latF := StrToFloat(StringReplace(Copy(LatLong,0,ansipos(',',LatLong)-1),'.',',',[rfIgnoreCase, rfReplaceAll]));
lonF := StrToFloat(StringReplace(Copy(LatLong,ansipos(',',LatLong)+1,11),'.',',',[rfIgnoreCase, rfReplaceAll]));
// Creates a TStringList from a Memo to send (around half a sec of proccessing)
bufferlist := TStringList.Create;
bufferlist.Add('h-023.64086400000,-046.57425900000 99999999 0300 0301 0001 test|123 test');
for J := 0 to Memo1.Lines.Count-2 do
begin
if ((abs(latF-StrToFloat(StringReplace(Copy(Memo1.Lines[J],2,16),'.',',',[rfIgnoreCase, rfReplaceAll]))) < 0.1) and (abs(lonF-StrToFloat(StringReplace(Copy(Memo1.Lines[J],19,16),'.',',',[rfIgnoreCase, rfReplaceAll]))) < 0.1)) then
bufferlist.Add(Memo1.Lines[J]);
end;
///////// Start to send
for i := 0 to bufferlist.Count-1 do
begin
AContext.Connection.IOHandler.WriteLn(bufferlist[i]);
end;
AContext.Connection.IOHandler.WriteLn('@'); // Send '@' to the client to say the list is over
bufferlist.Free;
end;
except
if Assigned(bufferlist) then bufferlist.Free;
Exit;
end;
end;
Since all the connections comes from 3G/4G phones I assume that some of them end bad and this is causing the problem, so what am I doing wrong in this code?
There is something I can do for solving this problem or at least improve it?
Your server's OnExecute is swallowing all raised exceptions. When the client disconnects, the next socket I/O performed on that client's socket will raise an exception, which you are catching and discarding. So the server will not know the client has disconnected, and will continue to fire the OnExecute event. And since your server is set to MaxConnections=1, a new client cannot connect to the server until the previous client has been fully released. Your OnExecute code would have to call AContext.Connection.Disconnect() directly, or raise an uncaught exception, to release the client thread.
Simple rule of thumb - don't swallow exceptions! If you catch an exception you don't know how to handle, you should re-raise it, as it may be handled higher up in the call stack. In terms of TIdTCPServer, never swallow an Indy exception that is derived from EIdException, let the server handle it. Your use of a try/except just to free bufferlist should be replaced with a try/finally instead.
In fact, I would suggest getting rid of the TStringList for gathering the response data, as it is not actually helping you, it is hindering your server's performance. The client will not receive any response from the server until the entire TStringList has been built in full, and that may cause timeouts on the client side. It would be better to send each line of text to the client as it is being created, so the client knows the server is actually doing something and is not dead/frozen.
Another problem I see with your OnExecute code is that it is directly accessing a UI control (a TMemo) without synchronizing with the UI thread. TIdTCPServer is a multi-threaded component, its events are fired in worker threads. You must synchronize with the UI thread when accessing UI controls in a worker thread.
And lastly, your overuse of Copy() and StringReplace() is a major eye-sore and makes the code hard to maintain in general.
Try something more like this instead:
procedure IdTCPServer1Connect(AContext: TIdContext);
begin
// do this assignment one time, not on every OnExecute loop iteration
AContext.Connection.IOHandler.DefStringEncoding := IndyTextEncoding_UTF8;
end;
procedure IdTCPServer1Execute(AContext: TIdContext);
var
s: string;
latF, lonF: Double;
fmt: TFormatSettings;
lines: TStringList;
j: Integer;
begin
s := AContext.Connection.IOHandler.ReadLn;
if s = '' then Exit;
fmt := TFormatSettings.Create;
fmt.DecimalSeparator := '.';
latF := StrToFloat(Fetch(s, ','), fmt);
lonF := StrToFloat(s, fmt);
AContext.Connection.IOHandler.WriteLn('h-023.64086400000,-046.57425900000 99999999 0300 0301 0001 test|123 test');
lines := TStringList.Create;
try
TThread.Synchronize(nil,
procedure
begin
lines.Assign(Memo1.Lines);
end
);
for J := 0 to lines.Count-2 do
begin
s := lines[J];
if (abs(latF-StrToFloat(Copy(s, 2, 16), fmt)) < 0.1) and (abs(lonF-StrToFloat(Copy(s, 19, 16), fmt)) < 0.1) then
AContext.Connection.IOHandler.WriteLn(s);
end;
finally
lines.Free;
end;
AContext.Connection.IOHandler.WriteLn('@'); // Send '@' to the client to say the list is over
end;
Now, on the client side, what you are doing is generally fine (though I would suggest using a worker thread instead of a timer in the main UI thread), however I will say that you do not need to call InputBuffer.Clear() for a graceful disconnect, only an abnormal disconnect (ie, move it into your except handler), and you should not be calling CloseGracefully() at all.
Also, since you are terminating the response with a unique delimiter, I would suggest that once the client detects the server's response starting to arrive, it should use TIdIOHandler.Capture() to read the entire response in one go, instead of calling ReadLn() to read each individual line on each timer event. That will greatly simplify and speed up how fast the client receives the complete response.
procedure Timer6Timer(Sender: TObject);
var
response: TStringList;
begin
with IdDownloadClient do
begin
try
if IOHandler.InputBufferIsEmpty then
begin
IOHandler.CheckForDataOnSource(0);
IOHandler.CheckForDisconnect;
if IOHandler.InputBufferIsEmpty then Exit;
end;
response := TStringList.Create;
try
// The server send an '@' to say the complete response has been sent
IOHandler.Capture(response, '@', False);
// use response as needed...
finally
response.Free;
end;
except
IOHandler.InputBuffer.Clear;
end;
Disconnect;
end;
Timer6.Enabled := False;
end;