Does this delphi code has memory leak?

Question

There is two service written in Delphi. One service is run at server and the other service are run at clients. These two services communicate to exchange data between client and server. In this case server service ask clients to send data, However after a day or so the allocated memory on server exceeds it's physical size. After some research I found something is wrong in these two functions below.

Do I have memory leak in following code ?

Server Service:

function TServerMethods.LogInMngr_GetAllUsers(): TList<TLogInClass>;
Var
  C: TDBXCommand;
Begin
  C := CreateCommand('LogInMngr_GetAllUsers');
  try
    C.ExecuteUpdate;
    Result := GetValue<TList<TLogInClass>>(C.Parameters[RP]);
  finally
    C.Free;
  end;
end;

Client Service:

function TLogInMngrClass.GetAllUsers(status: TLogInStatus = lisUnknown): TList<TLogInClass>;
var
  LogIn: TLogInItem;
  LogInTemp: TLogInClass;
  UsedProg:  TUsedItem;
begin
  Result := TList<TLogInClass>.Create;

  for LogIn in LogInList do
    if (status = lisUnknown) or (LogIn.Status = Integer(status)) then
      for UsedProg in LogIn.UsedProgList do
      begin
        LogInTemp:=TLogInClass.Create(LogIn.ClientID, LogIn.ComputerName, LogIn.UserName, '', '');
        LogInTemp.LogInTime := DateTimeToFileTime(UsedProg.AccessTime);
        LogInTemp.Status := LogIn.Status;
        LogInTemp.ProgName := UsedProg.ItemName;
        LogInTemp.ProgVersion := UsedProg.ItemInfo;

        Result.Add(LogInTemp);
      end;
end;

Data is in class TLogInClass and the result is returned as a generic list of class TLogInClass using TDSServer.

"*Does this delphi code has memory leak?*" Probably yes but there's no evidence of that in the code you have posted. There's no evidence of how `TList` is freed. Also it should be a `TObjectList` which owns values but it's just a guess like answers will be — fantaghirocco, Aug 30 '16 at 07:41
You do not show how both result `TList` are deallocated. Please add this information. — LU RD, Aug 30 '16 at 07:47

score 0 · Answer 1 · answered Aug 30 '16 at 07:46

0

You can find easily with this...

Memory Leak Notification in Delphi

  ReportMemoryLeaksOnShutdown := DebugHook <> 0;

answered Aug 30 '16 at 07:46

Steve88

2,366
3
24
43

That won't find all leaks. Won't find leaks where memory should be released during execution that is only released at shutdown. Important for long running processes. – David Heffernan Aug 30 '16 at 07:52

John Kouraklis · Accepted Answer · 2016-09-01T05:14:56.550

As others have mentioned, you don't show how the "result" of the function at the client side is memory-managed.

I suspect the GetAllUsers as the client is called multiple times, which in turn fires the Create many times and this adds up to the memory consumption.

Apart from this, I always find difficult to manage objects return by functions. It is hard to determine how and when the "result" should be freed and whether they are freed.

At the moment, the server-side function is not "aware" who is responsible to manage the result.

I would rewrite that function to a procedure with a parameter like this:

procedure TServerMethods.LogInMngr_GetAllUsers (var userList: TObjectList<TLogInClass>);
begin
  if not assigned(userList) then 
    raise Exception.Create('userList not instantiated');
  ...
end;

and then call the procedure this:

var     
  tmpUsers: TObjectList<TLogInClass>;
begin
 tmpUsers:=TObjectList<TLogInClass>.Create;
 try
   ServerMethods.LogInMngr_GetAllUsers(tmpUsers); <--- or whatever you call the ServerMethods class
   .....
 finaly
   tmpUsers.Free;
 end;

end;

This way you can clearly manage the life-cycle of the lists.

Perhaps I don't follow your design or miss some bits but I think you get the point of using procedures instead of functions to manage objects.

Functions that return new imstances are fine so long as they include Create in the name to indicate that they create new instances. Treat them as you treat constructors. Your so called improvement leaks in case of exception. You need try finally. — David Heffernan, Aug 31 '16 at 06:15
@DavidHeffernan: I didn't say they are not fine. I said it is easier to manager IMO. Thanks for the point. I edited the code now and it is "double" so called improved — John Kouraklis, Sep 01 '16 at 05:16

Does this delphi code has memory leak?

2 Answers2