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.