Thread-safe removal of destruction callbacks in web scopes

Closes gh-23117

Author: jhoeller

spring-web/src/main/java/org/springframework/web/context/ContextCleanupListener.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2019 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.
@@ -54,22 +54,26 @@ public void contextDestroyed(ServletContextEvent event) {
 
 
 	/**
-	 * Find all ServletContext attributes which implement {@link DisposableBean}
-	 * and destroy them, removing all affected ServletContext attributes eventually.
-	 * @param sc the ServletContext to check
+	 * Find all Spring-internal ServletContext attributes which implement
+	 * {@link DisposableBean} and invoke the destroy method on them.
+	 * @param servletContext the ServletContext to check
+	 * @see DisposableBean#destroy()
 	 */
-	static void cleanupAttributes(ServletContext sc) {
-		Enumeration<String> attrNames = sc.getAttributeNames();
+	static void cleanupAttributes(ServletContext servletContext) {
+		Enumeration<String> attrNames = servletContext.getAttributeNames();
 		while (attrNames.hasMoreElements()) {
 			String attrName = attrNames.nextElement();
 			if (attrName.startsWith("org.springframework.")) {
-				Object attrValue = sc.getAttribute(attrName);
+				Object attrValue = servletContext.getAttribute(attrName);
 				if (attrValue instanceof DisposableBean) {
 					try {
 						((DisposableBean) attrValue).destroy();
 					}
 					catch (Throwable ex) {
-						logger.error("Couldn't invoke destroy method of attribute with name '" + attrName + "'", ex);
+						if (logger.isWarnEnabled()) {
+							logger.warn("Invocation of destroy method failed on ServletContext " +
+									"attribute with name '" + attrName + "'", ex);
+						}
 					}
 				}
 			}

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2017 the original author or authors.
+ * Copyright 2002-2019 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.
@@ -188,18 +188,17 @@ public void setAttribute(String name, Object value, int scope) {
 	public void removeAttribute(String name, int scope) {
 		if (scope == SCOPE_REQUEST) {
 			if (isRequestActive()) {
-				this.request.removeAttribute(name);
 				removeRequestDestructionCallback(name);
+				this.request.removeAttribute(name);
 			}
 		}
 		else {
 			HttpSession session = getSession(false);
 			if (session != null) {
 				this.sessionAttributesToUpdate.remove(name);
 				try {
-					session.removeAttribute(name);
-					// Remove any registered destruction callback as well.
 					session.removeAttribute(DESTRUCTION_CALLBACK_NAME_PREFIX + name);
+					session.removeAttribute(name);
 				}
 				catch (IllegalStateException ex) {
 					// Session invalidated - shouldn't usually happen.

spring-web/src/main/java/org/springframework/web/context/support/ServletContextScope.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2017 the original author or authors.
+ * Copyright 2002-2019 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.
@@ -78,8 +78,10 @@ public Object get(String name, ObjectFactory<?> objectFactory) {
 	public Object remove(String name) {
 		Object scopedObject = this.servletContext.getAttribute(name);
 		if (scopedObject != null) {
+			synchronized (this.destructionCallbacks) {
+				this.destructionCallbacks.remove(name);
+			}
 			this.servletContext.removeAttribute(name);
-			this.destructionCallbacks.remove(name);
 			return scopedObject;
 		}
 		else {
@@ -89,7 +91,9 @@ public Object remove(String name) {
 
 	@Override
 	public void registerDestructionCallback(String name, Runnable callback) {
-		this.destructionCallbacks.put(name, callback);
+		synchronized (this.destructionCallbacks) {
+			this.destructionCallbacks.put(name, callback);
+		}
 	}
 
 	@Override
@@ -112,10 +116,12 @@ public String getConversationId() {
 	 */
 	@Override
 	public void destroy() {
-		for (Runnable runnable : this.destructionCallbacks.values()) {
-			runnable.run();
+		synchronized (this.destructionCallbacks) {
+			for (Runnable runnable : this.destructionCallbacks.values()) {
+				runnable.run();
+			}
+			this.destructionCallbacks.clear();
 		}
-		this.destructionCallbacks.clear();
 	}
 
 }