Prevent accessing the session in ServletRequestAttributes.updateAccessedSessionAttributes() if no session attributes need to be updated [SPR-13950]

[spring-projects-issues]

Mathieu Lachance opened SPR-13950 and commented

Hi,

we are currently using Wildfly with pessimistic web cache and we are running into contention issue in ServletRequestAttributes.updateAccessedSessionAttributes() due to the session access attempt:

this.session = this.request.getSession(false);

Could we add a simple guard to make sure we do not try to access the session if there's nothing in the sessionAttributesToUpdate map? i.e.:

	/**
	 * Update all accessed session attributes through {@code session.setAttribute}
	 * calls, explicitly indicating to the container that they might have been modified.
	 */
	@Override
	protected void updateAccessedSessionAttributes() {
                // proposed improvement request: skip if nothing has changed
                if (this.sessionAttributesToUpdate.isEmpty()) {
                    // nothing to do
                    return;
                }

		// Store session reference for access after request completion.
		this.session = this.request.getSession(false);
		// Update all affected session attributes.
		if (this.session != null) {
			try {
				for (Map.Entry<String, Object> entry : this.sessionAttributesToUpdate.entrySet()) {
					String name = entry.getKey();
					Object newValue = entry.getValue();
					Object oldValue = this.session.getAttribute(name);
					if (oldValue == newValue && !isImmutableSessionAttribute(name, newValue)) {
						this.session.setAttribute(name, newValue);
					}
				}
			}
			catch (IllegalStateException ex) {
				// Session invalidated - shouldn't usually happen.
			}
		}
		this.sessionAttributesToUpdate.clear();
	}

Let me know if I'm missing something.

Thanks,

---

Affects: 4.2.4

Referenced from: commits 8495fcf

[spring-projects-issues]

Juergen Hoeller commented

Good point. I've rolled such a not-empty check into 4.3, for both ServletRequestAttributes and PortletRequestAttributes. To be backported to 4.2.5 in time for its release on Feb 22.

Juergen

[spring-projects-issues]

Juergen Hoeller commented

Unfortunately, I need to reopen this one since we cannot access the session after request completion anymore: That's also what the code-level comment above the getSession call indicates. After a request completed, some asynchronous processing might still try to refer to session attributes and therefore need the original request's HttpSession handle... and we cannot rely on obtaining it on demand from the HttpServletRequest itself after completion.

I suppose you are running into lock contention in WildFly's HttpServletRequest implementation? Or does WildFly make some assumptions based on whether getSession has been called for a given request? Any details on the effect that you are seeing there?

Juergen

[spring-projects-issues]

Mathieu Lachance commented

Whenever you try access a session (i.e. httpRequest.getSession) in pessismistic mode, Wildfly will try to acquire a lock on that session to prevent any usage in any other threads. That lock will get release at the very end of the request.

We currently have multiple urls in our code that do not rely at all on the session and we would like to make sure we do not introduce others to avoid increased contention which, in our case, often result in TimeoutException.

By reading the actual code, I'm still not sure that I understand why eagerly exiting the method when there's no attribute to update will cause an issue.
Could you describe in details a possible scenario (ex: thread 1 .... thread 2 ..., etc.) so I get better why this is an issue and maybe figure out how this could be fixed.

Thank you very much Juergen,

[spring-projects-issues]

Juergen Hoeller commented

The problem is that our updateAccessedSessionAttributes does two things: It of course processes actual session attribute updates but it also stores the obtained HttpSession reference for potential later use in other threads that spawn from the original request's thread. If we don't assign the session field there, attempts to access the session from the given ServletRequestAttributes will fail after the original request completed. See the protected ServletRequestAttributes.getSession method for that late-access code path.

Now, it is debatable whether it is a good idea to try to obtain any session attributes after the original request's completion. I suppose that might only ever happen if asynchronous tasks spawned from the original request, where it is not guaranteed that they kick in during the actual request's lifetime. From my perspective, it is a corner case but unfortunately explicitly supported at this point.

That said, we might get away with simply calling HttpServletRequest.getSession on demand even after the request's completion in such cases. This is very old code anyway, predating Servlet 3.0; I'm not sure it is still relevant to begin with. We could simply use a stored session reference if we happen to have it and fall back to trying HttpServletRequest.getSession otherwise, which as a consequence would allow updateAccessedSessionAttributes to skip session access completely if there are no attributes to update.

We can certainly revise that arrangement for 4.3 and see what feedback we get. My main concern at this point is whether do this for 4.2.5 under those circumstances.

Juergen

[spring-projects-issues]

Mathieu Lachance commented

Glad to heard this seems to be an "outdated corner case".
We will go ahead by temporary patching our Spring jar while waiting for 4.3.x release. So far, all the use cases we had seemed to be fine with that small addition.

Should I watch another ticket to track the "arrangement" for 4.3 ?

Thanks!

[spring-projects-issues]

Juergen Hoeller commented

Resolved for 4.3 now, always storing the latest session reference that's been in actual use but not doing any session access anymore if there are no session attributes to update. This should cover pessimistic getSession assumptions as in WildFly now, while still allowing for post-completion access to the session if it has been in use before (or if a request.getSession call still works, as a fallback, even after the request completed).