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,9 +104,7 @@ public class ServletRequestAttributes extends AbstractRequestAttributes {
 				try {
 					Object value = session.getAttribute(name);
 					if (value != null) {
-						synchronized (this.sessionAttributesToUpdate) {
-							this.sessionAttributesToUpdate.put(name, value);
-						}
+						this.sessionAttributesToUpdate.put(name, value);
 					}
 					return value;
 				}
@@ -129,9 +127,7 @@ public class ServletRequestAttributes extends AbstractRequestAttributes {
 		}
 		else {
 			HttpSession session = getSession(true);
-			synchronized (this.sessionAttributesToUpdate) {
-				this.sessionAttributesToUpdate.remove(name);
-			}
+			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);
-				}
+				this.sessionAttributesToUpdate.remove(name);
 				try {
 					session.removeAttribute(name);
 					// Remove any registered destruction callback as well.
@@ -228,24 +222,22 @@ 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()) {
-						String name = entry.getKey();
-						Object newValue = entry.getValue();
-						Object oldValue = this.session.getAttribute(name);
-						if (oldValue == newValue) {
-							this.session.setAttribute(name, newValue);
-						}
+		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) {
+						this.session.setAttribute(name, newValue);
 					}
 				}
-				catch (IllegalStateException ex) {
-					// Session invalidated - shouldn't usually happen.
-				}
 			}
-			this.sessionAttributesToUpdate.clear();
+			catch (IllegalStateException ex) {
+				// Session invalidated - shouldn't usually happen.
+			}
 		}
+		this.sessionAttributesToUpdate.clear();
 	}
 
 	/**

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,18 +115,14 @@ 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);
-						}
+						this.globalSessionAttributesToUpdate.put(name, value);
 					}
 					return value;
 				}
 				else {
 					Object value = session.getAttribute(name);
 					if (value != null) {
-						synchronized (this.sessionAttributesToUpdate) {
-							this.sessionAttributesToUpdate.put(name, value);
-						}
+						this.sessionAttributesToUpdate.put(name, value);
 					}
 					return value;
 				}
@@ -150,15 +146,11 @@ 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);
-				}
+				this.globalSessionAttributesToUpdate.remove(name);
 			}
 			else {
 				session.setAttribute(name, value);
-				synchronized (this.sessionAttributesToUpdate) {
-					this.sessionAttributesToUpdate.remove(name);
-				}
+				this.sessionAttributesToUpdate.remove(name);
 			}
 		}
 	}
@@ -176,15 +168,11 @@ 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);
-					}
+					this.globalSessionAttributesToUpdate.remove(name);
 				}
 				else {
 					session.removeAttribute(name);
-					synchronized (this.sessionAttributesToUpdate) {
-						this.sessionAttributesToUpdate.remove(name);
-					}
+					this.sessionAttributesToUpdate.remove(name);
 				}
 			}
 		}
@@ -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) {
+		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,8 +263,12 @@ public class PortletRequestAttributes extends AbstractRequestAttributes {
 					}
 				}
 			}
-			this.globalSessionAttributesToUpdate.clear();
+			catch (IllegalStateException ex) {
+				// Session invalidated - shouldn't usually happen.
+			}
 		}
+		this.sessionAttributesToUpdate.clear();
+		this.globalSessionAttributesToUpdate.clear();
 	}
 
 	/**