[keycloak-dev] ClientSessions may never be removed

[Bill Burke]

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
>

-- 
Bill Burke
JBoss, a division of Red Hat
http://bill.burkecentral.com