SessionRepositoryFilter - session loaded an additional time from repository due to premature local state reset

[sudhirtumati]

Describe the bug
We are using the spring-session library with the Redis repository. I observed that an additional HGETALL call is being made to Redis. Removing this additional call in an environment with a high user base and high TPS would be beneficial.

To Reproduce
Any HTTP request that updates existing session results in the below list of calls to Redis

Pre-processing - load session from the repository
HGETALL

Post-processing - update session state in the repository
EXISTS
HMSET
PEXPIREAT
HGETALL - additional call that can be avoided

Expected behavior
Session loaded only once from the repository per request

Possible solution
Current implementation of SessionRepositoryRequestWrapper.commitSession() method:

	private void commitSession() {
		HttpSessionWrapper wrappedSession = getCurrentSession();
		if (wrappedSession == null) {
			if (isInvalidateClientSession()) {
				SessionRepositoryFilter.this.httpSessionIdResolver.expireSession(this, this.response);
			}
		}
		else {
			S session = wrappedSession.getSession();
			clearRequestedSessionCache();
			SessionRepositoryFilter.this.sessionRepository.save(session);
			String sessionId = session.getId();
			if (!isRequestedSessionIdValid() || !sessionId.equals(getRequestedSessionId())) {
				SessionRepositoryFilter.this.httpSessionIdResolver.setSessionId(this, this.response, sessionId);
			}
		}
	}

clearRequestedSessionCache() - clears session details cached locally
getRequestedSessionId() - loads session from repository again

Moving clearRequestSessionCache() call to the end as shown below would prevent the additional call

	private void commitSession() {
		HttpSessionWrapper wrappedSession = getCurrentSession();
		if (wrappedSession == null) {
			if (isInvalidateClientSession()) {
				SessionRepositoryFilter.this.httpSessionIdResolver.expireSession(this, this.response);
			}
		}
		else {
			S session = wrappedSession.getSession();
			SessionRepositoryFilter.this.sessionRepository.save(session);
			String sessionId = session.getId();
			if (!isRequestedSessionIdValid() || !sessionId.equals(getRequestedSessionId())) {
				SessionRepositoryFilter.this.httpSessionIdResolver.setSessionId(this, this.response, sessionId);
			}
			clearRequestedSessionCache();
		}
	}

I am not sure whether the proposed change would have any side effects. Could someone please check and let me know?

[sudhirtumati]

Integration tests are failing in my local environment. Hence created PR #2132 to trigger the build pipeline and verify test results.

As I am the first-time contributor to this project, can someone please approve the workflow to run?

[vpavic]

Hi @sudhirtumati. This is something that has been reported and discussed several times:

• SessionRepositoryFilter's getRequestedSession is called unnecessarily. #1424
• Use cached requestedSession info if available #1578
• SessionRepositoryFilter.SessionRepositoryRequestWrapper.commitSession() triggers extraneous call to SessionRespository.findById(...) #1731
• [master] Avoid extraneous call to SessionRespository.findById(...) in SessionRepositoryRequestWrapper.commitSession() #1732
• Eliminate unnecessary sessionRepository::findById #1909

So this issue is basically a duplicate. Searching issue tracker before opening issues is generally a good practice. 🙂

Having said that, since this is a concern frequently requested by the community but at the same time a somewhat sensitive concern, we'll try to take a closer look at for 3.0.

Taking a step back, from Redis commands I see that you are using RedisIndexedSessionRepository that has been the default so far. Are you aware that since 2.2 release Spring Session also offers RedisSessionRepository, that is significantly less chatty in terms of communication with Redis (therefore more performant) with tradeoff being that it doesn't support session events and fetching sessions by index?

This session repository implementation will become the default in Spring Session 3.0. Using it in 2.0 requires some manual configuration, but there's a sample in Spring Session codebase that shows how to do that.

[sudhirtumati]

Hi @vpavic, sorry for not searching existing issues before reporting this one. My issue is the same as #1731, and #1732. We can mark this as a duplicate.

We already switched to RedisSessionRepository and found that it is efficient compared to RedisIndexedSessionRepository. But this additional findById call still happens with RedisSessionRepository as well.

[sudhirtumati]

Duplicate of #1731 and #1732