Optimize session resolution in SessionRepositoryFilter

[cemo]

We are using JDBCSessionRepository and started to profile our application. On line 303 and 306 session is fetched twice:

spring-session/spring-session-core/src/main/java/org/springframework/session/web/http/SessionRepositoryFilter.java

Lines 303 to 313 in d97ad2c

	String requestedSessionId = getRequestedSessionId();
	if (requestedSessionId != null) {
	if (getAttribute(INVALID_SESSION_ID_ATTR) == null) {
	S session = getSession(requestedSessionId);
	this.requestedSessionIdValid = true;
	currentSession = new HttpSessionWrapper(session, getServletContext());
	currentSession.setNew(false);
	setCurrentSession(currentSession);
	return currentSession;
	}
	}

Here is the relevant getRequestedSessionId part.

spring-session/spring-session-core/src/main/java/org/springframework/session/web/http/SessionRepositoryFilter.java

Lines 354 to 361 in d97ad2c

	@Override
	public String getRequestedSessionId() {
	return SessionRepositoryFilter.this.httpSessionIdResolver
	.resolveSessionIds(this).stream()
	.filter(sessionId -> SessionRepositoryFilter.this.sessionRepository
	.findById(sessionId) != null)
	.findFirst().orElse(null);
	}

@vpavic I think this is a regression since I do not remember twice select before.

[cemo]

@rwinch @vpavic

I think this issue is very important since getRequestedSessionId is buggy now. This method is used in many places. For a simple request findById is called at least called 4 times.

[vpavic]

Thanks for the report @cemo.

Why do you think the SessionRepositoryRequestWrapper#getRequestedSessionId is buggy? It does what it has to do in order to accommodate the changes to HttpSessionIdResolver (the successor to HttpSessionStrategy). For background on those changes see #275, #362 and #906.

That being said, I see your point with multiple retrieval of session as a part of SessionRepositoryRequestWrapper#getSession(boolean). I'll see what can we do to optimize that.

[cemo]

Sorry @vpavic. I did not mean buggy. In fact it has some performance issues. I think it can cache session in getRequestedSessionId as well.

[vpavic]

Thanks for clarification @cemo. I think the SessionRepositoryRequestWrapper#getRequestedSessionId itself is fine, however some of its usages within the SessionRepositoryFilter could be optimized to avoid double retrieval from the session repository. That's the direction I'm currently looking into, will post back here when I have further updates.

[cemo]

Thanks @vpavic. Please let me know after you optimized it. I can test for you.

[cemo]

Please also note that it was called not only beginning of the request. At the end of request lifecycle it was called again multiple times.

org.springframework.session.web.http.SessionRepositoryFilter.SessionRepositoryRequestWrapper#commitSession

commitSession session is called twice.

[vpavic]

@cemo I've pushed some WIP changes in #1050. This should optimize things a bit by not using SessionRepositoryRequestWrapper#getRequestedSessionId internally, where possible, and instead reusing retrieved session.

It seems that SessionRepositoryRequestWrapper#commitSession is a bit harder to avoid.

If you can give these changes a shot, that would be great.

[cemo]

@vpavic I will give a try but I think that we have a fundamental mistake.

from javax.servlet.http.HttpServletRequest#getRequestedSessionId:

    /**
     * Returns the session ID specified by the client. This may
     * not be the same as the ID of the current valid session
     * for this request.
     * If the client did not specify a session ID, this method returns
     * <code>null</code>.
     *
     * @return		a <code>String</code> specifying the session
     *			ID, or <code>null</code> if the request did
     *			not specify a session ID
     *
     * @see     #isRequestedSessionIdValid
     */

I think validating it by fetching from repository may not be correct behaviour. This might be better to be addressed in isRequestedSessionIdValid.

I will also others implementation as well and let you know.

[cemo]

As I guessed, it has fixed behaviour. Please see imlementation:

https://github.com/eclipse/jetty.project/blob/977fb521a0be5a5440434b840533ba9e006966ce/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java#L2148-L2151

And

It has set only from SessionHandler in without checking its validity.

https://github.com/eclipse/jetty.project/blob/jetty-9.4.x/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java#L1605-L1690

Please let me know what do you think?

[vpavic]

The thing is that HttpServletRequest might provide multiple session id's, which we support via HttpSessionIdResolver#resolveSessionIds. How else would you align that with HttpServletRequest#getRequestedSessionId and select one of the provided id's? I'd again like to point you to #275 and #362 which provide background for this.

[cemo]

I see your point. I have double checked Jetty's implementation. They addressed same concern by setting session and session id at the same time. Here is the relevant part https://github.com/eclipse/jetty.project/blob/21365234f8cf036829f7a03e5c7b89a9572444f9/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java#L1605-L1690

They are iterating all cookies and after a successful match they break the loop and than trying to set at the end of the method both session and session id.

[cemo]

Dear @vpavic, is there a due date for 2.0.3?

[vpavic]

We don't have any dates set yet, we'll update the milestones page when we do.

[cemo]

@vpavic I think that this issue has not resolved by this commit. Should I open another one? I had written my findings at 7513753#r28863608

[vpavic]

Should I open another one?

Please do as both closed issues and commit comments are not ideal places to discuss further improvements in this area.