Replaced Map synchronization with ConcurrentHashMap to avoid session access deadlocks

Issue: SPR-10436

spring-web/src/main/java/org/springframework/web/context/request/ServletRequestAttributes.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2013 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,8 +16,8 @@
 
 package org.springframework.web.context.request;
 
-import java.util.HashMap;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpSession;
 
@@ -50,7 +50,7 @@ public class ServletRequestAttributes extends AbstractRequestAttributes {
 
 	private volatile HttpSession session;
 
-	private final Map<String, Object> sessionAttributesToUpdate = new HashMap<String, Object>();
+	private final Map<String, Object> sessionAttributesToUpdate = new ConcurrentHashMap<String, Object>(1);
 
 
 	/**
@@ -104,10 +104,8 @@ public class ServletRequestAttributes extends AbstractRequestAttributes {
 				try {
 					Object value = session.getAttribute(name);
 					if (value != null) {
-						synchronized (this.sessionAttributesToUpdate) {
 						this.sessionAttributesToUpdate.put(name, value);
 					}
-					}
 					return value;
 				}
 				catch (IllegalStateException ex) {
@@ -129,9 +127,7 @@ public class ServletRequestAttributes extends AbstractRequestAttributes {
 		}
 		else {
 			HttpSession session = getSession(true);
-			synchronized (this.sessionAttributesToUpdate) {
 			this.sessionAttributesToUpdate.remove(name);
-			}
 			session.setAttribute(name, value);
 		}
 	}
@@ -147,9 +143,7 @@ public class ServletRequestAttributes extends AbstractRequestAttributes {
 		else {
 			HttpSession session = getSession(false);
 			if (session != null) {
-				synchronized (this.sessionAttributesToUpdate) {
 				this.sessionAttributesToUpdate.remove(name);
-				}
 				try {
 					session.removeAttribute(name);
 					// Remove any registered destruction callback as well.
@@ -228,7 +222,6 @@ public class ServletRequestAttributes extends AbstractRequestAttributes {
 		// Store session reference for access after request completion.
 		this.session = this.request.getSession(false);
 		// Update all affected session attributes.
-		synchronized (this.sessionAttributesToUpdate) {
 		if (this.session != null) {
 			try {
 				for (Map.Entry<String, Object> entry : this.sessionAttributesToUpdate.entrySet()) {
@@ -246,7 +239,6 @@ public class ServletRequestAttributes extends AbstractRequestAttributes {
 		}
 		this.sessionAttributesToUpdate.clear();
 	}
-	}
 
 	/**
 	 * Register the given callback as to be executed after session termination.

spring-webmvc-portlet/src/main/java/org/springframework/web/portlet/context/PortletRequestAttributes.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2013 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,8 +16,8 @@
 
 package org.springframework.web.portlet.context;
 
-import java.util.HashMap;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 import javax.portlet.PortletRequest;
 import javax.portlet.PortletSession;
 
@@ -59,9 +59,9 @@ public class PortletRequestAttributes extends AbstractRequestAttributes {
 
 	private volatile PortletSession session;
 
-	private final Map<String, Object> sessionAttributesToUpdate = new HashMap<String, Object>();
+	private final Map<String, Object> sessionAttributesToUpdate = new ConcurrentHashMap<String, Object>(1);
 
-	private final Map<String, Object> globalSessionAttributesToUpdate = new HashMap<String, Object>();
+	private final Map<String, Object> globalSessionAttributesToUpdate = new ConcurrentHashMap<String, Object>(1);
 
 
 	/**
@@ -115,19 +115,15 @@ public class PortletRequestAttributes extends AbstractRequestAttributes {
 				if (scope == SCOPE_GLOBAL_SESSION) {
 					Object value = session.getAttribute(name, PortletSession.APPLICATION_SCOPE);
 					if (value != null) {
-						synchronized (this.globalSessionAttributesToUpdate) {
 						this.globalSessionAttributesToUpdate.put(name, value);
 					}
-					}
 					return value;
 				}
 				else {
 					Object value = session.getAttribute(name);
 					if (value != null) {
-						synchronized (this.sessionAttributesToUpdate) {
 						this.sessionAttributesToUpdate.put(name, value);
 					}
-					}
 					return value;
 				}
 			}
@@ -150,18 +146,14 @@ public class PortletRequestAttributes extends AbstractRequestAttributes {
 			PortletSession session = getSession(true);
 			if (scope == SCOPE_GLOBAL_SESSION) {
 				session.setAttribute(name, value, PortletSession.APPLICATION_SCOPE);
-				synchronized (this.globalSessionAttributesToUpdate) {
 				this.globalSessionAttributesToUpdate.remove(name);
 			}
-			}
 			else {
 				session.setAttribute(name, value);
-				synchronized (this.sessionAttributesToUpdate) {
 				this.sessionAttributesToUpdate.remove(name);
 			}
 		}
 	}
-	}
 
 	@Override
 	public void removeAttribute(String name, int scope) {
@@ -176,19 +168,15 @@ public class PortletRequestAttributes extends AbstractRequestAttributes {
 			if (session != null) {
 				if (scope == SCOPE_GLOBAL_SESSION) {
 					session.removeAttribute(name, PortletSession.APPLICATION_SCOPE);
-					synchronized (this.globalSessionAttributesToUpdate) {
 					this.globalSessionAttributesToUpdate.remove(name);
 				}
-				}
 				else {
 					session.removeAttribute(name);
-					synchronized (this.sessionAttributesToUpdate) {
 					this.sessionAttributesToUpdate.remove(name);
 				}
 			}
 		}
 	}
-	}
 
 	@Override
 	public String[] getAttributeNames(int scope) {
@@ -256,8 +244,8 @@ public class PortletRequestAttributes extends AbstractRequestAttributes {
 	@Override
 	protected void updateAccessedSessionAttributes() {
 		this.session = this.request.getPortletSession(false);
-		synchronized (this.sessionAttributesToUpdate) {
 		if (this.session != null) {
+			try {
 				for (Map.Entry<String, Object> entry : this.sessionAttributesToUpdate.entrySet()) {
 					String name = entry.getKey();
 					Object newValue = entry.getValue();
@@ -266,11 +254,6 @@ public class PortletRequestAttributes extends AbstractRequestAttributes {
 						this.session.setAttribute(name, newValue);
 					}
 				}
-			}
-			this.sessionAttributesToUpdate.clear();
-		}
-		synchronized (this.globalSessionAttributesToUpdate) {
-			if (this.session != null) {
 				for (Map.Entry<String, Object> entry : this.globalSessionAttributesToUpdate.entrySet()) {
 					String name = entry.getKey();
 					Object newValue = entry.getValue();
@@ -280,9 +263,13 @@ public class PortletRequestAttributes extends AbstractRequestAttributes {
 					}
 				}
 			}
-			this.globalSessionAttributesToUpdate.clear();
+			catch (IllegalStateException ex) {
+				// Session invalidated - shouldn't usually happen.
 			}
 		}
+		this.sessionAttributesToUpdate.clear();
+		this.globalSessionAttributesToUpdate.clear();
+	}
 
 	/**
 	 * Register the given callback as to be executed after session termination.