Add a SafeDeserializingSessionRepository

[rwinch]

I'm not sure about the name just yet, but we should have a SessionRepository implementation that can ignore specific set of exceptions to allow ignoring of deserialization exceptions (i.e. when a serialization UID changes). The implementation would look something like this, but would either allow for customizing the exceptions that are ignored or we would require updates to the existing implementations to ensure that they threw a consistent exception if deserialization failed (perfered):

public class SafeDeserializingSessionRepository<S extends ExpiringSession> implements SessionRepository<S> {
    private final SessionRepository<S> repository;

    public SafeDeserializingSessionRepository(SessionRepository<S> repository) {
        this.repository = repository;
    }

    public S getSession(String id) {
        try {
            return repository.getSession(id);
        } catch(SerializationException e) {
            delete(id);
            return null;
        }
    }

    public S createSession() {
        return repository.createSession();
    }

    public void save(S session) {
        repository.save(session);
    }

    public void delete(String id) {
        repository.delete(id);
    }
}

Relates to #280

[vpavic]

@rwinch What do you think about introducing some sort of error handling strategy to deal with this scenario? The default implementation would be no-op and user could plug-in the desired strategy into SessionRepository of their choice.

Regarding the approach you've outlined - what's your take on registering SessionRepository bean(s) with application context? IMO preferred approach would be to have a single SessionRepository in the application context, meaning only decorator instance would be registered. However, this would be an issue with some delegates since we have multiple SessionRepository implementations that use @Scheduled on their methods.

Furthermore, this decorator should probably be implementing FindByIndexNameSessionRepository, right?

[rwinch]

@vpavic Thank you for your thoughts.

To be honest I haven't completely thought this one out myself so your response is quite valuable in getting this moving forward.

What do you think about introducing some sort of error handling strategy to deal with this scenario? The default implementation would be no-op and user could plug-in the desired strategy into SessionRepository of their choice.

I like the decorator strategy because it can impact all implementations and all locations with a single piece of code.

Regarding the approach you've outlined - what's your take on registering SessionRepository bean(s) with application context? IMO preferred approach would be to have a single SessionRepository in the application context, meaning only decorator instance would be registered. However, this would be an issue with some delegates since we have multiple SessionRepository implementations that use @Scheduled on their methods.

I think we would have to register both implementations, but mark one of the beans as primary. This is a bit unfortunate side effect of Java Configuration not being as flexible as XML configuration.

Furthermore, this decorator should probably be implementing FindByIndexNameSessionRepository, right?

This is a good point.

[vpavic]

@rwinch Considering #546 will introduce new strategy for fetching sessions, this serialization handling decorator is IMO better suited to be applied to this new interface. That way we'll eliminate the need to have multiple SessionRepository registered with the app context. Your thoughts on this approach?

[rwinch]

@vpavic Thanks for your feedback. It sounds as though that solution would be limited to JDBC operations? I would prefer to allow this feature to support all repository implementations.

[vpavic]

@rwinch Sorry for the noise, somehow I managed to completely disregard the fact this should apply to all repository implementations. Still fresh off the vacation I guess... :confused:

So, assuming we go with the decorator implementing FindByIndexNameSessionRepository, would you just provide this implementation and let the users wrap it around the delegate themselves or should there be some support for applying it automatically?

[vpavic]

@rwinch I'll give this a look, hopefully in time to add it to 1.3.0.M3.

[vpavic]

I've proposed the solution for this in #646 - feedback is welcome.

[leccelecce]

@rwinch / @vpavic I hate to be "that guy" but any chance of an update on this ticket - either if it's likely to make its way into a release any time, or failing that what the recommended workaround to solve this problem is?

Right now, all we seem to be able to do is manually kill every active session via the Redis CLI every time we update the JVM or a related Spring class. This is pretty hideous in a multi-instance rolling update environment.

[sc-moonlight]

@spazbob We have the same issue. The solution depends on which versions you are using, but if you are using newer versions, then you can configure a SessionRepositoryCustomizer bean to override the serializer with something a bit safer.

So when the sessionid is looked up, if it can't be deserialized then it just returns null. Which will mean they end up with a new session. Be aware that depending on how your rollout works, you could kick out users more than once if they could bounce between new and old versions. On my list is to convert either the whole session to a JSON object, or change our code so the session contains the absolute minimum, and everything else for the user is stored in a Spring Cache (still backed by Redis), and set that serializer to use JSON. Both are a lot of analysis and work though.

private RedisSerializer<Object> safeJavaSerializer() {
    RedisSerializer<Object> java = RedisSerializer.java(getClass().getClassLoader());
    return new RedisSerializer() {
        @Override
        public byte[] serialize(Object o) throws SerializationException {
            return java.serialize(o);
        }

        // In case of a version change, we just evict the value from the cache.
        @Override
        public Object deserialize(byte[] bytes) throws SerializationException {
            try {
                return java.deserialize(bytes);
            } catch (SerializationException ignored) {
            }
            return null;
        }
    };
}

@Bean
public org.springframework.session.config.SessionRepositoryCustomizer custom(){
    return (SessionRepositoryCustomizer<RedisIndexedSessionRepository>) sessionRepository -> {
        RedisOperations<Object, Object> sessionRedisOperations = sessionRepository.getSessionRedisOperations();
        ((RedisTemplate)sessionRedisOperations).setHashValueSerializer(safeJavaSerializer());
    };
}

[dalbani]

Too bad this issue has been removed from the backlog. I'm sure lots of people regularly end up in a situation where sessions stored in the database "unexplicably" become invalid, for example due to an upgrade of the Spring libraries. In combination with https://github.com/spring-projects/spring-session/issues/1913 that has also stalled, the current state of affairs out of the box is suboptimal (tip for onlookers: use JSON serialization).

[joaquinjsb]

Too bad this issue has been removed from the backlog. I'm sure lots of people regularly end up in a situation where sessions stored in the database "unexplicably" become invalid, for example due to an upgrade of the Spring libraries. In combination with #1913 that has also stalled, the current state of affairs out of the box is suboptimal (tip for onlookers: use JSON serialization).

with JSON I had a problem instantiating the Authentication object, did you find any workaround?