-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SessionRepositoryFilter's getRequestedSession is called unnecessarily. #1424
Comments
I can confirm that this leads to the session being loaded from redis multiple times per request, which really should be avoided. |
Prevent fetching the session multiple times when checking whether the sessionId has to be sent to the client, by comparing values before clearing the requestedSessionCache. Resolves: spring-projects#1424
Prevent fetching the session multiple times when checking whether the sessionId has to be sent to the client, by comparing values before clearing the requestedSessionCache. Resolves: spring-projects#1424
Could somebody take a look at this? The fix suggested by @oxc is quite trivial and prevents hitting Redis multiple times per request. |
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 @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 |
@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. |
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 |
Hello! I think we're all expecting the feedback of @vpavic since quite some time. Is there a way to move the issue forward? |
Sorry for my late follow-up on this. The history behind the caching of the fetched session in 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. |
Any update on this? |
Any update guys? As a prove it's still the issue in Spring Session 2.6.1 I attached the sql queries executed for a single request....
I noticed that
Is this expected @vpavic ? |
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 ofRedisOperationsSessionRepository.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 containingjsp: include
,commitSession
is executed each timejsp: 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 byRedisOperationsSessionRepository.findById
exists in the session.commitSession
callsgetRequestedSessionId
,getRequestedSessionId
callsRedisOperationsSessionRepository.findById
viagetRequestedSession
.Because
RedisOperationsSessionRepository.findById
has become is a process that callsRedisOperationsSessionRepository.getSession(String, boolean)
, increase in the number of executions ofcommitSession
leads to an increase ofRedisOperationsSessionRepository.getSession (String, boolean)
This is
getRequestedSessionId
but due to the modification from Spring Session 2.1.1.RELEASE onwards,getRequestedSession
can be executed to updaterequestedSessionId
only whenrequestedSessionId
is null, by caching therequestedSessionId
.However, at the same time in
clearRequestedSessionCache
a process is added to assign null torequestedSessionId
as well and incommitSession
in order to executeclearRequestedSessionCache
before executingrequestedSessionId
it has become necessary to executegetRequestedSession
.spring-session/spring-session-core/src/main/java/org/springframework/session/web/http/SessionRepositoryFilter.java
Lines 357 to 363 in c5fc4b5
spring-session/spring-session-core/src/main/java/org/springframework/session/web/http/SessionRepositoryFilter.java
Lines 392 to 396 in c5fc4b5
spring-session/spring-session-core/src/main/java/org/springframework/session/web/http/SessionRepositoryFilter.java
Lines 229 to 248 in c5fc4b5
Since
requestedSessionId
is not changed during the request, it is better not to clear itIf the call of
getRequestedSession
is properly controlled fromgetRequestedSessionId
, even ifgetRequestedSessionId
is called fromcommitSession
bySessionCommittingRequestDispatcher.include (ServletRequest, ServletResponse)
the behavior of executingRedisOperationsSessionRepository.findById
every time will not occur . It is likely that the number ofRedisOperationsSessionRepository.getSession (String, boolean)
execution will be the same as before the version upgrade.The text was updated successfully, but these errors were encountered: