[keycloak-dev] Slight refactoring of InfinispanUserSessionProvider

[Marek Posolda]

In today's call with Stian and Hynek, we discussed the possible 
refactoring of InfinispanUserSessionProvider to be reliable in 
cluster/cross-dc environment.

Current implementation has tightly couple between the model 
(UserSessionAdapter) and underlying infinispan entity. For example when 
you call "userSessionModel.setLastSessionRefresh(123)" it directly 
delegates this call to infinispan entity and calls 
"entity.setLastSessionRefresh(123)" .

Some issues with this approach:

1) Possibility of lost updates in cluster - It can happen that there are 
more concurrent requests updating same userSession. There is a fix by 
Bill [1], which adds the thread-safety to UserSessionEntity and fixes 
the issue in local environment. However in cluster with more nodes, it 
can still happen that there is "lost update". I've created the JIRA [2] 
for this with details. In shortcut, what can happen is, that there is 
request1 processed on node1 and request2 processed on node2 in parallel. 
Both requests read the sessionEntity and then both perform concurrent 
update and commits their transaction. Then the latter update wins and 
overwrites the first update as the state when it read the entity didn't 
yet contain the first update.

2) Rollback doesn't work correctly. Basically when we do this:

{code}
userSession.setLastSessionRefresh(123);
session.transaction().rollback();
{code}

the keycloak transaction should be rolled-back and session in infinispan 
cache shouldn't be updated to lastSessionRefresh 123. However in current 
implementation, rollback doesn't work and entity is always updated in 
local environment and may be updated in cluster too (in case that 
calling node is the owner of session entity). That's because we are not 
using infinispan transaction API and calling 
"entity.setLastSessionRefresh(123)" directly updates the entity instance 
referenced by infinispan storage.

3) Cross-dc support with good performance

Ideal is to fix all issues and ensure consistency, but have the 
same/similar performance at the same time and not introduce additional 
uneccessary locking.


Possible solution:

- Every change to UserSessionModel won't delegate directly to infinispan 
sessionEntity as it's now. Instead it will track the change through 
changelogs/events, which are saved locally in our infinispan transaction 
(eg. AddClientToSessionEvent, LastSessionRefreshUpdateEvent).

- During Keycloak transaction commit are changelogs applied to 
sessionEntity. It may use atomic operation "cache.replace(key, oldValue, 
newValue)" and the pattern like this:

{code}
boolean replaced = false;

while (!replaced) {
     SessionEntity oldSession = cache.get("123");
     // Will update session entity based on the changes tracked in 
infinispan transaction
     SessionEntity updatedSession = applyChanges(oldSession);
     replaced = cache.replace("123", oldSession, updatedSession);
}
{code}

- In cross-dc environment are changes sent to another datacenter. That 
will receive changes (eg. AddClientToSessionEvent) and apply changes 
locally in the datacenter and update the sessionEntity in the 
infinispan. It shouldn't be a problem that those events are sent between 
datacenters through the asynchronous channel. The changes may not be 
guaranteed in same order in every DC, but all of them will be triggered 
and there won't be lost updates. So the cache "sessions" won't be 
directly replicated through the cross-dc, but instead the changes will 
be sent between datacenters.

What do you think? Can you see some issues with this approach?

Thanks,
Marek

[1] https://issues.jboss.org/browse/KEYCLOAK-4821
[2] https://issues.jboss.org/browse/KEYCLOAK-4898