Session.removeAttribute doesn't remove attribute from Redis

[aterleto]

Hello,

The implementation within org.springframework.session.data.redis.RedisOperationsSessionRepository puts the attribute, to be removed, back into Redis and nulls out the value. This breaks consistency with the cached session attributes and creates an orphan in the Redis database. Furthermore, upon fail-over if the session is re-hydrated back from Redis the attribute will once again be cached and available to the web app, however this time without its value.

@Override public void removeAttribute(String attributeName) { this.cached.removeAttribute(attributeName); this.putAndFlush(getSessionAttrNameKey(attributeName), null); }

Shouldn't the entire attribute (key and value) be removed from Redis?

[vpavic]

Thanks for the report @aterleto.

This breaks consistency with the cached session attributes and creates an orphan in the Redis database. Furthermore, upon fail-over if the session is re-hydrated back from Redis the attribute will once again be cached and available to the web app, however this time without its value.

Can you clarify this part - I'm not sure I understand what do you mean? Namely it's not clear to me how hash keys that are nulled out could show as session attributes again. Could you put together a sample that demonstrates the problematic behavior?

Also please take care to provide some minimum information when reporting issues, such as Spring Session release that you use, and any related configuration.

[aterleto]

Thanks @vpavic.

I'm using the following versions - <spring.version>5.1.4.RELEASE</spring.version><spring.session.redis.version>2.1.2.RELEASE</spring.session.redis.version>

I included screenshots to show that the attribute key remains in Redis after the attribute is "removed". Please ignore the garbage created by the JdkSerializationRedisSerializer.

Before attribute 'A' is removed

After attribute 'A' is removed

Regarding failover, I ran a test and it seems that my assertion was incorrect. I should have ran a test before adding that assumption to the case, please disgard. I didn't anticipate there would be logic somewhere which doesn't add keys with NULL values into the app-level Spring Session cache. Seems a bit opinionated to me and actually reinforces the main question - why not remove the key from Redis along with the value? If the attribute is removed and never added back into Spring Session's cache then why would you leave a reference to the attribute (orphan) in the distributed cache i.e.. Redis?

[vpavic]

@eleftherias I'm not a fan of this being addressed at the expense of performance i.e. with additional round trips to Redis. Was there a real issue with attributes being nulled out in Redis?

If you insist on this approach, note that RedisSessionRepository isn't aligned and now behaves differently in this regard.

[eleftherias]

Thanks for pointing that out @vpavic!

The issue was raised again by a user whose integration tests were failing, because they were expecting the attribute to be removed from Redis. I took at look at the Hazelcast and JDBC implementations and they are both removing the attribute as well. I think nulling out the attribute instead causes confusion. I'm open to other suggestions on how to remove it that may have less impact on performance.

[vpavic]

That fact that attributes were nulled out is IMO an internal implementation detail of Redis backed session stores, and the most optimal approach in terms of keeping number of calls to data store at minimum (which is also a performance consideration). I don't think we should be comparing different data stores from the perspective of how they store sessions internally as all of those data stores have certain pros and cons.

I believe the problem should be looked at from functional perspective, and it doesn't seem to me there's any defect related to the fact that attribute are being nulled out instead of deleted.

The issue was raised again by a user whose integration tests were failing, because they were expecting the attribute to be removed from Redis.

This sounds to me like they're testing our implementation, rather than their own code. Was there an additional issue created here on GitHub? If yes, could you point me to it?

[aterleto]

@vpavic The decision to null out the attribute's value instead of removing the entire attribute is made by the Spring Data implementation (org.springframework.session.data.redis.RedisOperationsSessionRepository) and not anyone else's application. I originally opened this issue because I believed upon application restart there would be an orphan but found that there is extra logic added into Spring Session to filter out attributes with null values. So basically there was some decision made to optimize on writes instead of reads which I'm not sure I agree with considering this is Redis however it technically didn't impact my original concern so I left it alone. Long story short, this isn't related to Redis but an implementation decision. My assumption is Spring Session's implementation has to generalize based on the limitation of most data stores so this was applied to Redis as well.

[eleftherias]

@vpavic Sorry for the delayed response.

The conversation was not on GibHub, but the main question was this.

Looking around it seems like spring-session-data-redis is the only spring-session-data-[something] scheme with this behavior. Is there a reason why it’s implemented this way?

I agree with you that there is no defect, but I don't have a good answer to why spring-session-data-redis is the only one that has this performance optimization.

Should we be doing something similar in the other data stores to keep the number of calls to a minimum? And if not, what is different about Redis that makes us need to limit the calls?

[vpavic]

Good timing on bringing this up - I was just preparing the PR to revert this yesterday so that it doesn't fall through the cracks while waiting on feedback on a closed issue (see #1709). Especially since the 2.4.0 release is getting close.

As I tried to explain in my previous comment, each data store we support has its specifics and we shouldn't try to align session repository implementation on basis of how session data is materialized in different data stores. Besides, the API what we're committed to is HttpSession/WebSession (and of course our own org.springframework.session.Session, but that's not primary used-facing API) and from that perspective this issue is IMO an invalid report.

So in this particular case we're using the HMSET command to update the session in Redis, and for attributes that have been deleted we store them as null which, coupled with logic in session repository implementations (i.e. RedisIndexedSessionRepository, RedisSessionRepository and ReactiveRedisSessionRepository) handles it so that the mentioned session APIs are respected.

Should we be doing something similar in the other data stores to keep the number of calls to a minimum?

To my best knowledge we're already doing this in all session repository implementations this repo handles (Redis, JDBC, Hazelcast). Can you point out one example where you think that's not the case?

[eleftherias]

To my best knowledge we're already doing this in all session repository implementations this repo handles (Redis, JDBC, Hazelcast). Can you point out one example where you think that's not the case?

@vpavic Looking at JDBC, we are using a separate operation to delete the session attributes, rather than combining it with the update operation and updating them to null.

This seems like a similar situation to Redis, but in the Redis implementation we are using one command for both update and delete.

[vpavic]

A session with 4 session attributes is stored in Redis as a single hash, whereas in a relational database it's stored as 5 records (1 for the core session data and 4 for each of the attributes) spread across 2 tables.

If there's a change to session's attributes you have to go to the attributes table not matter what so there's not much to spare there as you simply have to execute several SQL statements in order to update session.

With Redis, update boils down to a single HMSET. I really can't see a valid reason to support any further network roundtrips on top of that one.

[vpavic]

I forgot to mention one more thing - don't forget that number of records in session attributes table directly impacts performance. If by rather than combining it with the update operation and updating them to null you meant to say you'd update the session table to set it to null, rather than deleting it, that would mean you would be accumulating records in session attributes table. I doubt you'd reduce the number of network round trips but you would definitely increase the number of records in there.

This is simply not the case with Redis because there's always a single record (hash) in there that represents the session.

Like I stated numerous times in this thread, each data store has its pros and cons and the session repository implementations should be mindful of that, rather than directly comparing internal implementation aspects.

[eleftherias]

Thanks @vpavic, that makes sense.

I will go ahead and merge the PR you created and reopen this issue.

[vpavic]

Thanks @eleftherias.

Just to note - I don't think you should remove this from the 2.4.0-RC1 milestone and remove the labels as that release was already published.

[ParadiseWitch]

I think it needs to provide an additional delete method that removes attributes from Redis, in order to provide users with their own choice.