make sure the session is committed only once per request

[sadraskol]

When the session is queried by another filter through the request, any subsequent calls to jsp:include commits the session and fetches the session to do so. To avoid that, I forced commitSession to be called once. I ignore if this interacts with other parts of spring-session but it seems a reasonable solution.

You can find an example of this behavior in this small application: https://github.com/sadraskol/spring-session-jsp

solves #1424

[pivotal-issuemaster]

@sadraskol 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]

@sadraskol Thank you for signing the Contributor License Agreement!

[sadraskol]

These changes are not working properly: since the session is "committed" once, any subsequent changes on the session are discarded. I don't think this is an expected behavior.
I attempted to adapt my changes, but there is one operation that I don't understand: why do we need to call clearRequestedSessionCache when committing the session? I don't quite get its purpose in the event. My mental model tells me that we could get rid of it.

I think @vpavic mentionned a concurrency bug in the ordering of this method. Has it anything to do with it?

[sadraskol]

I implemented an alternative way to make it work : clear the cache only when the request is handled. I don't know if this works as intended though.

[sadraskol]

Hi there! Any news on this issue? I would like to understand the problems we're facing to help out.

[quaff]

@sadraskol Could you test #1910 against your case?

[vpavic]

I was taking a closer look at this, but upon inspecting the Servlet spec on include dispatch, it appears to me that commit session shouldn't even be invoked there as it potentially writes headers and therefore violates the spec.

@rwinch Let's discuss this on Monday to decide what we do for 3.0. I'm inclined to merge #1909 instead.

[sadraskol]

Sorry for my late response.
Indeed #1910 is much more clean than my PR in which i tried to have least side effect as possible.
I would like to see the test replicated to make sure it does work.

Unfortunately, I do not work in the company that encountered the problem... I'll forward them the PR and let them answer if they still have the issue.

[vpavic]

Thanks for following up @sadraskol.

My previous comment was a bit outdated. In the end, we went with #1909 (see also #2194) and that is a part of 3.0.0-RC1 release yesterday. Those changes should consistently eliminate one round trip to datastore to fetch the session.

Regarding #1910, we intend to revisit that at some point, but we want to make sure that if changes to session do happen after the response has been committed (and that's certainly possible), they would still get persisted.

Finally, thanks for your contributions to this topic, even though this PR didn't get merged in the end.