[keycloak-dev] ClientSessions may never be removed

[Marek Posolda]

I've fixed some issues, but just created JIRAs for the performance 
optimizations with fix version 1.X.
https://issues.jboss.org/browse/KEYCLOAK-801
https://issues.jboss.org/browse/KEYCLOAK-802

These are not the bugs, so I can look at them later, probably after 
release (will first finish setup of performance test, which will use 
bigger number of UserSessions + ClientSessions and then try search them).

Marek

On 30.10.2014 14:32, Bill Burke wrote:
> Looks good.
>
> On 10/30/2014 9:26 AM, Marek Posolda wrote:
>> 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
>> _______________________________________________
>> keycloak-dev mailing list
>> keycloak-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>