Adding expiration criteria when obtaining user/client sessions from store

[mhajas]

When obtaining sessions from the store, the first thing we do is check their expiration time. Then if the session is expired we are removing it. See code:

keycloak/model/map/src/main/java/org/keycloak/models/map/userSession/MapUserSessionProvider.java

Lines 84 to 89 in f11573e

	if (origEntity.getExpiration() <= Time.currentTime()) {
	if (Objects.equals(origEntity.getPersistenceState(), TRANSIENT)) {
	transientUserSessions.remove(origEntity.getId());
	}
	userSessionTx.delete(origEntity.getId());
	return null;

I am wondering, should we introduce a new EXPIRATION searchable field to save some network utilization? This way we wouldn't load expired sessions from the database at all.

[ahus1]

In this case loading an expired session happens only once, as it will be deleted in the next step. Most of the time the session is valid, or doesn't exist. If we don't delete it here, the deletion will happen in the reaper cleanup of that particular store that would then run longer than before as it will need to handle more sessions, might block other transactions, but it might be more efficient.

Assuming a concurrent environment, two concurrent calls might try to delete the same entity, with one deleting failing / getting a lock exception. Handling this in the reaper would avoid this.

Again assuming a concurrent environment, the current setup will delete the session once the first Keycloak instance sees it as expired. With the new approach different Keycloak instances might have a little time difference, and the Keycloak instance that has a clock in the past would still return it as valid, even if another Keycloak instance with the current time has previously seen this session as invalid.

Please let me know if I get too hypothetical here... :-)

To conclude, I'd rather leave it as is and not change it as I don't see a significant improvement here.

[sguilhen]

I had to actually re-read the third paragraph a few times to be able to parse everything ;) but in general I agree that while this could be an improvement I don't think it is one worth the time to get done.

[hmlnarik]

I am wondering, should we introduce a new EXPIRATION searchable field to save some network utilization? This way we wouldn't load expired sessions from the database at all.

Yes, we should, to be consistent across data stores. Datastores that can leverage expiration are auth / client / user sessions, and events, and potentially also other areas like users or clients as outlined in #10370.

Especially in case of big datasets (which e.g. events often are), we need to effectively restrict the number of returned records and avoid deleting them one by one or turning them into model.

[mhajas]

I agree, I created an issue here: #11947

[vramik]

@hmlnarik, @mhajas

Thank you.

I wonder how the removal of expired objects from database would be done. It should be prevented to let dataset grow to large numbers due to expired objects.

Should there be some kind of scheduled task which would be executed periodically to remove those? It might be the most beneficial to have this kind of task outside of keycloak.

Should we document that expired objects are left in the database? Or is it store implementation responsibility to maintain it?

[ahus1]

While running this outside Keycloak instances that serve traffic would be beneficial, those JVM instances would need to have the DB credentials, and would need to join the same caches (?). Eventually those can be jobs started with the same Keycloak binary, but without serving traffic.

Until then, I assume we'll have that job running inside a regular Keycloak, and at some time in the future it somewhere else.

[mhajas]

Related question. Should we leverage Infinispan lifespan for entities that are expirable? For events, I created the ExpirableEntity interface. This way we can check if entity we create is expirable and if yes we can put it into Infinispan with lifespan.

After this change, we should be able to disregard the EXPIRATION field from all HotRod queries as expiration is solved automatically. WDYT?

[hmlnarik]

Yes, this is a way to achieve it that leverages specific store features.