[master] Avoid extraneous call to SessionRespository.findById(...) in SessionRepositoryRequestWrapper.commitSession()

[pferraro]

Resolves #1731
Clearing the requested session cache after we've written the session ID to the response avoid the need for a redundant lookup of the session.

[pivotal-issuemaster]

@pferraro Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-issuemaster]

@pferraro Thank you for signing the Contributor License Agreement!

[eleftherias]

Thanks for the PR @pferraro!

Could you add Closes gh-1731 to the commit message so that it will automatically close the issue you created?

[pferraro]

@eleftherias Done.

[vpavic]

The lookup might seem redundant when one considers only what happens within the scope of this method (or even JVM), but have in mind that one of the core challenges Spring Session solves is session management in a cluster.

With that in mind, this needs to be looked at having in mind the scenarios with concurrent requests. The session id validity check that guards the invocation of HttpSessionIdResolver#setSessionId needs to know the up-to-date state, otherwise I think we could expose ourself to a number of subtle issues in scenarios involving concurrent requests, especially since things have been this way since the early days of Spring Session. Hence the unit test that clearly describes that 3 invocations are expected.

An important bit of context here is that SessionRepository#save has historically been implemented in such a way to be as performant as possible, specifically in this case it's relevant that invoking #save on a session that was already deleted from the store by some other request will result in a no-op. This is done to prevent reads within the implementation of #save. I believe that different implementations of SessionRepository aren't consistent here but at the same time there isn't any specific contract here to adhere to. That's why I opened #1277 a while ago.

So the change proposed here IMO shouldn't be done without considering a more substantial review of the whole arrangement.

[pferraro]

The lookup might seem redundant when one considers only what happens within the scope of this method (or even JVM), but have in mind that one of the core challenges Spring Session solves is session management in a cluster.

Concurrent requests to a given Session by multiple cluster members are forbidden by section 7.7.2 of the Servlet specification:

Within an application marked as distributable, all requests that are part of a session must be handled by one JVM at a time.

Thus, we only need to be concerned with concurrent requests for a given Session within a single JVM.

With that in mind, this needs to be looked at having in mind the scenarios with concurrent requests. The session id validity check that guards the invocation of HttpSessionIdResolver#setSessionId needs to know the up-to-date state...

Assuming that the state referenced by a Session returned by SessionRepository.findById(...) for a given request is, by definition, up-to-date until the corresponding Session#save completes, can we not simply reorder this logic such that we perform the session id validity check before Session#save?

I believe that different implementations of SessionRepository aren't consistent here but at the same time there isn't any specific contract here to adhere to. That's why I opened #1277 a while ago.

It seems to me that a SessionRepository implementation that persists an invalid session is incorrect, no?

[vpavic]

Thus, we only need to be concerned with concurrent requests for a given Session within a single JVM.

I'm afraid the excerpt you're quoting isn't really applicable to modern environments, as nowadays we typically have a round robin based load-balancers that distribute requests evenly between the nodes in a cluster. Spring Session is exactly here to address that scenario so that users don't have to resort to anti-patterns like sticky sessions (which is something Servlet spec seems to suggest there).

can we not simply reorder this logic such that we perform the session id validity check before Session#save?

Logically, the concern I'm expressing here would still be present, because #save precedes #setSessionId. Historically, we've seen many issues reported due to concurrency scenarios and that's why I'm hesitant to make any changes that could lead to subtle issues. We need to review the overall approach here and that looks like a 3.0 target to me.

It seems to me that a SessionRepository implementation that persists an invalid session is incorrect, no?

Yes, I agree with you there and that's exactly why I opened #1277.

[pferraro]

Logically, the concern I'm expressing here would still be present, because #save precedes #setSessionId. Historically, we've seen many issues reported due to concurrency scenarios and that's why I'm hesitant to make any changes that could lead to subtle issues. We need to review the overall approach here and that looks like a 3.0 target to me.

Can you describe a scenario where the proposed change is problematic - that is not already problematic using the current logic?

[lnxmad]

Any update on this?

[quaff]

@pferraro @lnxmad Could you try #1909?

[rwinch]

Closing in favor of gh-1909