[keycloak-dev] ClientSessions may never be removed

[Marek Posolda]

Few more points when looking at this:

* I wonder if we can optimize removeExpiredUserSessions(realm) a bit? 
For now it's called from periodic cleaner thread (if I not count unit 
test) separately for each realm. And specifically for 'mem' provider it 
needs to iterate over all UserSessions and ClientSessions for each call. 
So for example if we have 4 realms, then periodic cleaner needs to 
iterate 4 times over UserSessions and ClientSessions. Similarly  for 
infinispan, there are 4 MapReduce calls, which could be also a bit 
expensive. I wonder if it worth to change signature to either:
"removeExpiredUserSessions()" or 
"removeExpiredUserSessions(List<RealmModel> realms)" - both cases will 
handle cleanup of all the realms. This would mean that for 'mem' we need 
to iterate just once and for infinispan, we might have just 1 MapReduce 
call.

My assumption is that in highly concurrent envs could be count of 
UserSessions and ClientSessions quite big (also 95% of people will 
likely use either 'mem' or 'infinispan' provider for this, so worth 
optimization imo?

* For InfinispanUserSessionProvider, isn't better for performance if we 
use separate cache for UserSessions and separate for ClientSessions? 
Having all in one means that MapReduce needs to filter either all 
UserSessions if you are looking for ClientSessions and viceversa. Only 
place where you can use both in same MapReduce is 
"removeUserSessions(realm)" which is likely not called often

* InfinispanUserSessionProvider is using "loginFailures" cache for login 
failures, but this one is not defined in 
DefaultInfinispanConnectionProviderFactory and also in docs for 
infinispan subsystem integration (defining cache in standalone-ha.xml). 
I believe that this should be distributable or replicated cache too 
(invalidation won't work here). Should I create separate jira for that?

thoughts?
Marek

On 29.10.2014 19:29, Stian Thorgersen wrote:
>
> ----- Original Message -----
>> From: "Marek Posolda" <mposolda at redhat.com>
>> To: "Stian Thorgersen" <stian at redhat.com>
>> Cc: "keycloak dev" <keycloak-dev at lists.jboss.org>
>> Sent: Wednesday, 29 October, 2014 7:26:05 PM
>> Subject: Re: [keycloak-dev] ClientSessions may never be removed
>>
>> +1
>>
>> For mem we seem to be doing it too here:
>> https://github.com/keycloak/keycloak/blob/master/model/sessions-mem/src/main/java/org/keycloak/models/sessions/mem/MemUserSessionProvider.java#L197
>>
>> However it looks to me that there is bug in it. It's checking
>> ClientSessions without associated UserSession (which is ok to me as
>> those associated with UserSessionModel were cleaned previously), but the
>> bug is that it's not checking realm. So if realm 'foo' has idleTimeout
>> 30 secs, then it will cleanup all ClientSessions older than 30 seconds,
>> even from different realms...
> You're right, I miss-read the code ;)
>
>> Marek
>>
>> On 29.10.2014 19:03, Stian Thorgersen wrote:
>>> Looks like it's only Mongo and JPA that's doing this, while both mem and
>>> Infinispan are not.
>>>
>>> I reckon we just fix it for mem and Infinispan, there's not really any need
>>> for two separate methods.
>>>
>>> ----- Original Message -----
>>>> From: "Marek Posolda" <mposolda at redhat.com>
>>>> To: "Stian Thorgersen" <stian at redhat.com>, "keycloak dev"
>>>> <keycloak-dev at lists.jboss.org>
>>>> Sent: Wednesday, 29 October, 2014 5:28:10 PM
>>>> Subject: Re: [keycloak-dev] ClientSessions may never be removed
>>>>
>>>> Right now we are already doing the cleanup of expired ClientSessions in
>>>> UserSessionProvider.removeExpiredUserSessions() for mem, jpa and mongo
>>>> providers.
>>>>
>>>> So it seems that only one missing is InfinispanUserSessionProvider.
>>>>
>>>> Maybe it's better to introduce new method on UserSessionProvider like
>>>> "removeExpiredClientSessions()" and move the removal of expired client
>>>> sessions there? Or we can keep as it is and just fix the infinispan
>>>> provider? Not sure which possibility is better.
>>>>
>>>> Marek
>>>>
>>>> On 29.10.2014 16:23, Stian Thorgersen wrote:
>>>>> As new client sessions are initially detached there's a chance they are
>>>>> never linked to a user session (for example user closes browser when
>>>>> login
>>>>> page is displayed). These client sessions are never removed. I reckon we
>>>>> need to have a similar garbage collection of client sessions as we do for
>>>>> user sessions.
>>>>>
>>>>> https://issues.jboss.org/browse/KEYCLOAK-788
>>>>> _______________________________________________
>>>>> keycloak-dev mailing list
>>>>> keycloak-dev at lists.jboss.org
>>>>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>