Spring-session: SessionRepositoryFilter's getRequestedSession is called unnecessarily.
When Spring Session used in the application was upgraded from 1.3.1.RELEASE to 2.1.3.RELEASE, performance was verified and it was confirmed that the CPU utilization has increased by about 10%.
When profiling was performed using the function -agentlib: hprof = cpu = samples, it was confirmed that the execution time of RedisOperationsSessionRepository.getSession (String, boolean) has increased after version upgrade.
The increase in the number of executions of RedisOperationsSessionRepository.getSession (String, boolean) is due to the following two updates.
Update point 1
After Spring Session 1.3.4.RELEASE, with implementation of SessionCommittingRequestDispatcher which introduced the change that when screen transition is made to a JSP page containing jsp: include, commitSession is executed each time jsp: include is executed.
Update point 2
getRequestedSessionId has been modified since Spring Session 2.0.0.RELEASE, getRequestedSessionId has been changed to check whether the session ID obtained from cookie by RedisOperationsSessionRepository.findById exists in the session.
commitSession calls getRequestedSessionId, getRequestedSessionId calls RedisOperationsSessionRepository.findById via getRequestedSession.
Because RedisOperationsSessionRepository.findById has become is a process that calls RedisOperationsSessionRepository.getSession(String, boolean), increase in the number of executions of commitSession leads to an increase of RedisOperationsSessionRepository.getSession (String, boolean)
This is getRequestedSessionId but due to the modification from Spring Session 2.1.1.RELEASE onwards, getRequestedSession can be executed to update requestedSessionId only when requestedSessionId is null, by caching the requestedSessionId.
However, at the same time in clearRequestedSessionCache a process is added to assign null to requestedSessionId as well and in commitSession in order to execute clearRequestedSessionCache before executing requestedSessionId it has become necessary to execute getRequestedSession.
Since requestedSessionId is not changed during the request, it is better not to clear it
If the call of getRequestedSession is properly controlled from getRequestedSessionId, even if getRequestedSessionId is called from commitSession by SessionCommittingRequestDispatcher.include (ServletRequest, ServletResponse) the behavior of executing RedisOperationsSessionRepository.findById every time will not occur . It is likely that the number of RedisOperationsSessionRepository.getSession (String, boolean) execution will be the same as before the version upgrade.
hosoitk
All 8 comments
I can confirm that this leads to the session being loaded from redis multiple times per request, which really should be avoided.
oxc
on 7 Feb 2020
Could somebody take a look at this? The fix suggested by @oxc is quite trivial and prevents hitting Redis multiple times per request.
theigl
on 3 Mar 2020
We tried to solve this issue as suggested by @oxc, but it does not positively solve the issue. When including multiple jsp in the same call, the first include does not call redis but subsequent include do.
If i understand the introduction of this feature on its commit commitSession is called to disallow writes on header after an include. Yet, I don't quite get why commitSession is used. As multiple includes can happen in the same request, clearRequestedSessionCache will clear the cache for subsequent includes. I think something like the following should suffice to write only once the header:
@Override
public void include(ServletRequest request, ServletResponse response) throws ServletException, IOException {
if (!SessionRepositoryRequestWrapper.this.responseHeaderWritten) {
SessionRepositoryRequestWrapper.this.commitSession();
SessionRepositoryRequestWrapper.this.responseHeaderWritten = true;
}
this.delegate.include(request, response);
}
We are handling large jsp, we have to use jsp:include to be able to compile our jsp. As a result, spring-session pretty much DDOSing our redis instances.
@vpavic I see you are busy with #1732 which is related but is there a way to avoid this problem? Did I miss something? Is there a way to help you solve the issue?
sadraskol
on 28 Jan 2021
@sadraskol Thanks for the detailed analysis. If you could put together a simplified sample that illustrates the problem that would be the first step. The sample is to make it easier for the team to understand the issue in context.
It sounds like you have a proposal for a solution too. The second step would be putting together a solution. Your sample will demonstrate that the issue is fixed. The someone from the team can iterate on your PR.
rwinch
on 28 Jan 2021
Thanks @rwinch for the proposal, I create the simplest example I could come up with: https://github.com/sadraskol/spring-session-jsp
I am opening a PR next, overall I feel like the problem is that commitSession should not be called twice in the same request
sadraskol
on 29 Jan 2021
It seems like this is the change that added the invalidation 6d027900ee3a1567a5bd95c028b2be16ea503a2c
I'm not exactly sure all the history here. What are your thoughts @vpavic
rwinch
on 5 Feb 2021
Hello! I think we're all expecting the feedback of @vpavic since quite some time. Is there a way to move the issue forward?
sadraskol
on 26 Feb 2021
Sorry for my late follow-up on this.
The history behind the caching of the fetched session in SessionRepositoryFilter has been discussed recently in another PR - see #1732 and specifically in this comment.
In this specific case, I do see the problem as quite a serious one performance-wise, because as you explained @sadraskol the data store gets hit quite a lot. I'll do my best to review your PR by the end of this week.
vpavic
on 3 Mar 2021
Related issues
isalnikov
路
8Comments
abincai
路
6Comments
ysjiang4869
路
4Comments
ghost
路
7Comments
YLombardi
路
10Comments
Most helpful comment
Could somebody take a look at this? The fix suggested by @oxc is quite trivial and prevents hitting Redis multiple times per request.