Replies: 3 comments 5 replies
-
|
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. |
Beta Was this translation helpful? Give feedback.
-
|
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. |
Beta Was this translation helpful? Give feedback.
-
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. |
Beta Was this translation helpful? Give feedback.
-
|
I agree, I created an issue here: #11947 |
Beta Was this translation helpful? Give feedback.
-
|
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? |
Beta Was this translation helpful? Give feedback.
-
|
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. |
Beta Was this translation helpful? Give feedback.
-
|
Related question. Should we leverage Infinispan lifespan for entities that are expirable? For events, I created the After this change, we should be able to disregard the |
Beta Was this translation helpful? Give feedback.
-
|
Yes, this is a way to achieve it that leverages specific store features. |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
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
I am wondering, should we introduce a new
EXPIRATIONsearchable field to save some network utilization? This way we wouldn't load expired sessions from the database at all.Beta Was this translation helpful? Give feedback.
All reactions