Inside TROInMemorySessionManager.Destroy there is a call to ClearSessions(False) so that all current session objects are destroyed, with the intent on not leaking memory.
However, when there are expired sessions but the check interval has not expired yet, those session objects will NOT be freed and memory leaks ensue.
This is because TROInMemorySessionManager.DoGetAllSessions and TROInMemorySessionManager.DoGetAllSessionsIDs have the following code inside:
if CheckSessionIsExpired(l_currSession) then Continue;
This is totally undesirable because ClearSessions(False) is specifically called with the intent on not checking the expiration status of the session objects contained in the manager.
I have removed the four similar lines (2 per method) and now no longer have memory leaks when closing down my server.
This has been seen in version 10.0.0.1581, maybe it’s already been fixed in a later version.
I’m trying to investigate this case and I can’t get how it calls CheckSessionIsExpired:
TROInMemorySessionManager.Destroy has
ClearSessions(False);
TROCustomSessionManager.ClearSessions has
DoClearSessions(OnlyExpired);
TROInMemorySessionManager.DoClearSessions has
if OnlyExpired then begin
...
end
else begin
...
l_array := fSessionList.Values.ToArray;
for i := Low(l_array) to High(l_array) do
DeleteSession(l_array[i].SessionID, True); //<< typo, should have False, but it doesn't have any influence in `DoDeleteSession`
TROCustomSessionManager.DeleteSession has
DoDeleteSession(aSessionID, IsExpired);
TROInMemorySessionManager.DoDeleteSession has
if fSessionList.TryGetValue(aSessionID, l_Session) then begin
fSessionList.Remove(aSessionID);
FreeOrDisposeOf(l_Session);
end;
In my version, there is this inside ClearSessions:
DoClearSessions(GetAllSessions, OnlyExpired);
So it receives the list returned by GetAllSessions which should not be filtered in any kind of way but it actually is.
Once it reaches TROInMemorySessionManager.DoClearSessions method, the aSessionIds: TROSessionIDs list is not complete, hence the leak.
However, it seems that the code has slightly changed as in what you show DoClearSessions does not seem to receive a session list anymore.
Here is what the code looks like in my version:
procedure TROInMemorySessionManager.DoClearSessions(aSessionIds: TROSessionIDs; OnlyExpired: Boolean);
var
i: Integer;
begin
if OnlyExpired then begin
for i := Low(aSessionIds) to High(aSessionIds) do
if CheckSessionIsExpired(aSessionIds[i]) then DeleteSession(aSessionIds[i], True);
end
else begin
for i := Low(aSessionIds) to High(aSessionIds) do
DeleteSession(aSessionIds[i], True);
end;
end;
Thanks for pointing this out, I did make some changes here, but it’s way older than the current subject at hand.
It was back in 2021 with this message “ClearSessions should not lock the manager during its entire call.” and the following changes:
Interestingly, I did not notice at the time that GetAllSessions is filtering out the expired sessions, which, to me, is definitely counter intuitive.
I mean, I had a look at TRODBSessionManager.DoGetAllSessions and it does not appear to be filtering anything out.
So even if the consequence of getting memory leaks is not there in the release code, there is, to me, still an inconsistency in calling a method to get all sessions and actually not getting them all.
It has no sense to manipulate with expired sessions or store events for them so we filter expired sessions. Expired sessions will be deleted automatically by timer.
if you want to change default logic, better to create descendant or helper method and manipulate with fSessionList directly.
So you consider that a public method called GetAllSessions should indeed not return all sessions? That’s kind of a weird naming then, especially considering that the ClearSessions public method takes a parameter to specifically tell whether or not a filtering is done.
Yes, a new parameter would be ideal indeed, but I personally would not use a default value for it but rather use the deprecated keyword on the original method, like this:
procedure GetAllSessions(Dest: TStringList); overload; deprecated 'use procedure GetAllSessions(Dest, True)';
procedure GetAllSessions(Dest: TStringList; aExcludeExpired: Boolean); overload;
function GetAllSessions: TROSessionIDs; overload; deprecated 'use function GetAllSessions(True)';
function GetAllSessions(aExcludeExpired: Boolean): TROSessionIDs; overload;
Sure, this requires a bit more code, but it has the benefit of informing users of that public method has a new overload and that they should carefully review their usage. This would make sure they understand that historically GetAllSessions excludes expired sessions and that they might want to use a value for that parameter different from the current default.
