Save session on Redis RedisSession.saveDelta

[oassuncao]

I'm using Spring Session version 1.0.2.RELEASE and I found a problem when RedisSession save delta

If you add a new value on session (RedisSession) the session save correctly on Redis, because we have this code below:

        public void setAttribute(String attributeName, Object attributeValue) {
            cached.setAttribute(attributeName, attributeValue);
            delta.put(getSessionAttrNameKey(attributeName), attributeValue);
        }

But if in other request I get a object in this session, change some property of this object, the session on redis is not updated.

The problem is when we create RedisSession passing MapSession, this map is setter on variable "cached" but the values of "cached" doesn't pass to "delta"

Example to reproduce the problem:
POJO

public class NamePOJO implements Serializable {
    private String name;

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }
}

Controller

    ...
    @RequestMapping(value = "/changeSession/{value}")
    public void changeOnSession(@PathVariable(value  = "value") String value, HttpServletRequest request) throws IOException {
        final String sessionName = "SESSION_NAME";

        NamePOJO oldVersion;
        if (request.getSession().getAttribute(sessionName) == null) {
            oldVersion = new NamePOJO();
            oldVersion.setName(value);
            request.getSession().setAttribute(sessionName, oldVersion);
        }

        oldVersion = (NamePOJO)request.getSession().getAttribute(sessionName);

        log.info(String.format("Old name %s new name %s", oldVersion.getName(), value));
        oldVersion.setName(value);
    }
    ...

1* Request (/changeSession/Test)
Output: Old name Test new name Test
2* Request (/changeSession/Changed)
Output: Old name Test new name Changed
3* Request (/changeSession/Changed)
Output: Old name Test new name Changed

On the 3* request, the old name doesn't changed

[rwinch]

Hello @oassuncao and thank you for reporting this issue!

You are right that this is an issue, but it is deliberate. The reasoning is

• We want to limit the number of writes to the data store
• More importantly, we need to reduce race conditions. Consider the following:
  • Assume we have a Session with the attribute of username
  • Thread1 - starts to process a request to get a protected resource. The username attribute is present in the session.
  • Thread2 - processes a request to log the user out (remove the username attribute).
  • Thread2 - The username attribute is removed from the session and Redis is updated to reflect the username is removed
  • Thread1 - the request finishes and the Session is updated to include all of the original Session attributes. This means the username is now set in the Session which indicates the user is logged in. However, the user should be logged out.

The current approach handles the race condition scenario well by ensuring only the things that have changed are updated. The updates you have proposed would cause this scenario to fail.

I do understand your desire to fix the issue you mentioned. However, as far as I know the underlying contract of HttpSession does not guarantee that a session attribute will be updated unless the setAttribute method is invoked again. If you are using the default HttpSession implementation with many containers it does work (because everything is in memory). This would be similar to if you were using Spring Session's MapSessionRepsitory with a HashMap. However, if you use a servlet container's HttpSession clustered implementation the scenario mentioned here would also fail.

This is not to say that this is not a desired feature for some. In fact, the issue has been brought up before. For example #133 However, I think we need to make this a strategy that users can select since there are tradeoffs to each approach.

[oassuncao]

Hello @rwinch

I understand your point of view about race conditions, but we have the same problem if you use default implementation of HttpSession (Ex Tomcat).

I think we can make a possibility to do this (strategy), that users can implement or select a implementation to do this

[rwinch]

@oassuncao Thanks for the response. I agree that this would be a nice additional strategy.

Mostly wanted to point out that we don't want to remove our existing strategy

[shankarXXX]

@oassuncao did u get a chance to resolve the issue u posted in this thread?

[oassuncao]

@shankarXXX No, I disabled the Spring Session for now, I'm using the Tomcat Session and without cluster

[shankarXXX]

@oassuncao thanks for the reply.. i am disappointed as we were hoping to move our of containers with spring session without redoing the code completely.. looks like we need to do the refactor our code completely