Merge pull request #688 from JCgH4164838Gh792C124B5/localS2_62_ExecWaitCleanup
[WW-5312] Attempt to fix ExecuteAndWaitInterceptor inconsistent processing
diff --git a/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java b/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java
index dac8404..8fe5777 100644
--- a/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java
+++ b/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java
@@ -42,7 +42,7 @@
/**
- * Creates a new session map given a http servlet request. Note, ths enumeration of request
+ * Creates a new session map given a http servlet request. Note, the enumeration of request
* attributes will occur when the map entries are asked for.
*
* @param request the http servlet request object.
@@ -82,7 +82,7 @@
synchronized (session.getId().intern()) {
entries = null;
- Enumeration<String> attributeNamesEnum = session.getAttributeNames();
+ final Enumeration<String> attributeNamesEnum = session.getAttributeNames();
while (attributeNamesEnum.hasMoreElements()) {
session.removeAttribute(attributeNamesEnum.nextElement());
}
@@ -105,7 +105,7 @@
if (entries == null) {
entries = new HashSet<>();
- Enumeration<String> enumeration = session.getAttributeNames();
+ final Enumeration<String> enumeration = session.getAttributeNames();
while (enumeration.hasMoreElements()) {
final String key = enumeration.nextElement();
@@ -127,17 +127,21 @@
/**
* Returns the session attribute associated with the given key or <tt>null</tt> if it doesn't exist.
+ *
+ * <b>Note:</b> Must use the same signature as {@link java.util.AbstractMap#get(java.lang.Object)} to ensure the
+ * expected specialized behaviour is performed here (and not the generic ancestor behaviour).
*
* @param key the name of the session attribute.
* @return the session attribute or <tt>null</tt> if it doesn't exist.
*/
- public Object get(final String key) {
+ @Override
+ public Object get(final Object key) {
if (session == null) {
return null;
}
synchronized (session.getId().intern()) {
- return session.getAttribute(key);
+ return session.getAttribute(key != null ? key.toString() : null);
}
}
@@ -156,7 +160,7 @@
}
}
synchronized (session.getId().intern()) {
- Object oldValue = get(key);
+ final Object oldValue = get(key);
entries = null;
session.setAttribute(key, value);
return oldValue;
@@ -166,10 +170,14 @@
/**
* Removes the specified session attribute.
*
+ * <b>Note:</b> Must use the same signature as {@link java.util.AbstractMap#remove(java.lang.Object)} to ensure the
+ * expected specialized behaviour is performed here (and not the generic ancestor behaviour).
+ *
* @param key the name of the attribute to remove.
* @return the value that was removed or <tt>null</tt> if the value was not found (and hence, not removed).
*/
- public Object remove(final String key) {
+ @Override
+ public Object remove(final Object key) {
if (session == null) {
return null;
}
@@ -177,8 +185,9 @@
synchronized (session.getId().intern()) {
entries = null;
- Object value = get(key);
- session.removeAttribute(key);
+ final String keyAsString = (key != null ? key.toString() : null);
+ final Object value = get(keyAsString);
+ session.removeAttribute(keyAsString);
return value;
}
@@ -188,16 +197,21 @@
/**
* Checks if the specified session attribute with the given key exists.
*
+ * <b>Note:</b> Must use the same signature as {@link java.util.AbstractMap#containsKey(java.lang.Object)} to ensure the
+ * expected specialized behaviour is performed here (and not the generic ancestor behaviour).
+ *
* @param key the name of the session attribute.
* @return <tt>true</tt> if the session attribute exits or <tt>false</tt> if it doesn't exist.
*/
- public boolean containsKey(final String key) {
+ @Override
+ public boolean containsKey(final Object key) {
if (session == null) {
return false;
}
synchronized (session.getId().intern()) {
- return (session.getAttribute(key.toString()) != null);
+ final String keyAsString = (key != null ? key.toString() : null);
+ return (session.getAttribute(keyAsString) != null);
}
}
}
diff --git a/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java
index 7a022c9..8d48766 100644
--- a/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java
+++ b/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java
@@ -228,6 +228,7 @@
/* (non-Javadoc)
* @see com.opensymphony.xwork2.interceptor.MethodFilterInterceptor#doIntercept(com.opensymphony.xwork2.ActionInvocation)
*/
+ @Override
protected String doIntercept(ActionInvocation actionInvocation) throws Exception {
ActionProxy proxy = actionInvocation.getProxy();
String name = getBackgroundProcessName(proxy);
@@ -235,34 +236,40 @@
Map<String, Object> session = context.getSession();
HttpSession httpSession = ServletActionContext.getRequest().getSession(true);
- Boolean secondTime = true;
- if (executeAfterValidationPass) {
- secondTime = (Boolean) context.get(KEY);
- if (secondTime == null) {
- context.put(KEY, true);
- secondTime = false;
- } else {
- secondTime = true;
- context.put(KEY, null);
- }
- }
-
//sync on the real HttpSession as the session from the context is a wrap that is created
//on every request
synchronized (httpSession) {
- BackgroundProcess bp = (BackgroundProcess) session.get(KEY + name);
+ // State flag processing moved within the synchronization block, to ensure consistency.
+ Boolean secondTime = true;
+ if (executeAfterValidationPass) {
+ secondTime = (Boolean) context.get(KEY);
+ if (secondTime == null) {
+ context.put(KEY, true);
+ secondTime = false;
+ } else {
+ secondTime = true;
+ context.put(KEY, null);
+ }
+ }
+
+ final String bp_SessionKey = KEY + name;
+ BackgroundProcess bp = (BackgroundProcess) session.get(bp_SessionKey);
+
+ LOG.debug("Intercepting invocation for BackgroundProcess - session key: {}, value: {}", bp_SessionKey, bp);
//WW-4900 Checks if from a de-serialized session? so background thread missed, let's start a new one.
if (bp != null && bp.getInvocation() == null) {
- session.remove(KEY + name);
+ LOG.trace("BackgroundProcess invocation is null (remove key, clear instance)");
+ session.remove(bp_SessionKey);
bp = null;
}
if ((!executeAfterValidationPass || secondTime) && bp == null) {
+ LOG.trace("BackgroundProcess instance is null (create new instance) - executeAfterValidationPass: {}, secondTime: {}.", executeAfterValidationPass, secondTime);
bp = getNewBackgroundProcess(name, actionInvocation, threadPriority).prepare();
- session.put(KEY + name, bp);
- if (executor.isShutdown()) {
- LOG.warn("Executor is shutting down, cannot execute a new process");
+ session.put(bp_SessionKey, bp);
+ if (executor == null || executor.isShutdown()) {
+ LOG.warn("Executor is shutting down (or null), cannot execute a new process, invoke next ActionInvocation step and return.");
return actionInvocation.invoke();
}
executor.execute(bp);
@@ -271,6 +278,7 @@
}
if ((!executeAfterValidationPass || !secondTime) && bp != null && !bp.isDone()) {
+ LOG.trace("BackgroundProcess instance is not done (wait processing) - executeAfterValidationPass: {}, secondTime: {}.", executeAfterValidationPass, secondTime);
actionInvocation.getStack().push(bp.getAction());
final String token = TokenHelper.getToken();
@@ -297,7 +305,8 @@
return WAIT;
} else if ((!executeAfterValidationPass || !secondTime) && bp != null && bp.isDone()) {
- session.remove(KEY + name);
+ LOG.trace("BackgroundProcess instance is done (remove key, return result) - executeAfterValidationPass: {}, secondTime: {}.", executeAfterValidationPass, secondTime);
+ session.remove(bp_SessionKey);
actionInvocation.getStack().push(bp.getAction());
// if an exception occurred during action execution, throw it here
@@ -307,6 +316,7 @@
return bp.getResult();
} else {
+ LOG.trace("BackgroundProcess state fall-through (first instance, pass through), invoke next ActionInvocation step and return - executeAfterValidationPass: {}, secondTime: {}.", executeAfterValidationPass, secondTime);
// this is the first instance of the interceptor and there is no existing action
// already run in the background, so let's just let this pass through. We assume
// the action invocation will be run in the background on the subsequent pass through
@@ -394,7 +404,10 @@
@Override
public void destroy() {
- super.destroy();
- executor.shutdown();
+ try {
+ executor.shutdown();
+ } finally {
+ super.destroy();
+ }
}
}
diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcess.java b/core/src/main/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcess.java
index 8f56391..4223726 100644
--- a/core/src/main/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcess.java
+++ b/core/src/main/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcess.java
@@ -33,10 +33,10 @@
private final String threadName;
private final int threadPriority;
- private transient Thread processThread;
+ private transient Thread processThread;
//WW-4900 transient since 2.5.15
protected transient ActionInvocation invocation;
- protected transient Exception exception;
+ protected transient Exception exception;
protected String result;
protected boolean done;
@@ -64,9 +64,9 @@
afterInvocation();
} catch (Exception e) {
exception = e;
+ } finally {
+ done = true;
}
-
- done = true;
});
processThread.setName(threadName);
processThread.setPriority(threadPriority);
diff --git a/core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java b/core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java
index 8c292cd..6dc597e 100644
--- a/core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java
+++ b/core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java
@@ -36,6 +36,7 @@
import com.mockobjects.constraint.IsAnything;
import com.mockobjects.constraint.IsEqual;
import com.mockobjects.dynamic.Mock;
+import java.util.AbstractMap;
/**
@@ -47,24 +48,18 @@
public void testClearInvalidatesTheSession() throws Exception {
- List<String> attributeNames = new ArrayList<String>();
+ List<String> attributeNames = new ArrayList<>();
attributeNames.add("test");
attributeNames.add("anotherTest");
Enumeration<String> attributeNamesEnum = Collections.enumeration(attributeNames);
MockSessionMap sessionMap = new MockSessionMap((HttpServletRequest) requestMock.proxy());
- sessionMock.expect("getAttribute",
- new Constraint[] {
- new IsEqual("test")
- });
+ // Note: getAttribute() calls no longer expected after fix to ensure descendant (not ancestor) calls are made for
+ // the SessionMap Map methods (i.e. the overrides are called, as expected).
sessionMock.expect("setAttribute",
new Constraint[] {
new IsEqual("test"), new IsEqual("test value")
});
- sessionMock.expect("getAttribute",
- new Constraint[] {
- new IsEqual("anotherTest")
- });
sessionMock.expect("setAttribute",
new Constraint[] {
new IsEqual("anotherTest"), new IsEqual("another test value")
@@ -78,14 +73,6 @@
new Constraint[]{
new IsEqual("anotherTest")
});
- sessionMock.expect("getAttribute",
- new Constraint[] {
- new IsEqual("test")
- });
- sessionMock.expect("getAttribute",
- new Constraint[] {
- new IsEqual("anotherTest")
- });
sessionMap.put("test", "test value");
sessionMap.put("anotherTest", "another test value");
sessionMap.clear();
@@ -121,7 +108,7 @@
String key = "theKey";
Object value = new Object();
sessionMock.expectAndReturn("getAttribute", new Constraint[]{
- new IsEqual(key.toString())
+ new IsEqual(key)
}, value);
SessionMap sessionMap = new SessionMap((HttpServletRequest) requestMock.proxy());
@@ -134,7 +121,7 @@
Object value = new Object();
sessionMock.expect("getAttribute", new Constraint[]{new IsAnything()});
sessionMock.expect("setAttribute", new Constraint[]{
- new IsEqual(key.toString()), new IsEqual(value)
+ new IsEqual(key), new IsEqual(value)
});
SessionMap sessionMap = new SessionMap((HttpServletRequest) requestMock.proxy());
@@ -159,7 +146,7 @@
MockHttpServletRequest request = new MockHttpServletRequest();
String key = "theKey";
- Object someOtherKey = "someOtherKey";
+ String someOtherKey = "someOtherKey";
Object value = new Object();
SessionMap sessionMap = new SessionMap(request);
@@ -218,6 +205,79 @@
sessionMock.verify();
}
+ /**
+ * Attempt to detect any changes that would make the attribute handling for puts produce different results
+ * for the SessionMap and underlying HttpSession attributes.
+ *
+ * @throws Exception
+ */
+ public void testPutResultInSessionAttributes() throws Exception {
+ Object value = new Object();
+
+ //HttpSession httpSessionMock = ((HttpServletRequest) requestMock.proxy()).getSession(false);
+ HttpSession httpSessionMock = (HttpSession) sessionMock.proxy();
+ sessionMock.expectAndReturn("getAttribute", new Constraint[]{
+ new IsEqual("KEY")
+ }, null);
+ sessionMock.expect("setAttribute", new Constraint[]{
+ new IsEqual("KEY"), new IsEqual(value)
+ });
+ sessionMock.expectAndReturn("getAttribute", new Constraint[]{
+ new IsEqual("KEY")
+ }, value);
+ sessionMock.expectAndReturn("getAttribute", new Constraint[]{
+ new IsEqual("KEY")
+ }, value);
+
+ SessionMap sessionMap = new SessionMap((HttpServletRequest) requestMock.proxy());
+ AbstractMap<String, Object> abstractMap = (AbstractMap<String, Object>) sessionMap;
+ abstractMap.put("KEY", value);
+ assertEquals("Underlying HttpSession attribute does not match after SessionMap put ?", abstractMap.get("KEY"), httpSessionMock.getAttribute("KEY"));
+ sessionMock.verify();
+ }
+
+ /**
+ * Attempt to detect any changes that would make the attribute handling for removes produce different results
+ * for the SessionMap and underlying HttpSession attributes.
+ *
+ * @throws Exception
+ */
+ public void testRemoveResultInSessionAttributes() throws Exception {
+ Object value = new Object();
+ Object removedValue;
+
+ //HttpSession httpSessionMock = ((HttpServletRequest) requestMock.proxy()).getSession(false);
+ HttpSession httpSessionMock = (HttpSession) sessionMock.proxy();
+ sessionMock.expectAndReturn("getAttribute", new Constraint[]{
+ new IsEqual("KEY")
+ }, null);
+ sessionMock.expect("setAttribute", new Constraint[]{
+ new IsEqual("KEY"), new IsEqual(value)
+ });
+ sessionMock.expectAndReturn("getAttribute", new Constraint[]{
+ new IsEqual("KEY")
+ }, value);
+ sessionMock.expect("removeAttribute", new Constraint[]{
+ new IsEqual("KEY")
+ });
+ sessionMock.expectAndReturn("getAttribute", new Constraint[]{
+ new IsEqual("KEY")
+ }, null);
+ sessionMock.expectAndReturn("getAttribute", new Constraint[]{
+ new IsEqual("KEY")
+ }, null);
+
+ SessionMap sessionMap = new SessionMap((HttpServletRequest) requestMock.proxy());
+ AbstractMap<String, Object> abstractMap = (AbstractMap<String, Object>) sessionMap;
+ abstractMap.put("KEY", value);
+ removedValue = abstractMap.remove("KEY");
+ assertEquals("Removed attribute not equal to put attribute ?", value, removedValue);
+ assertNull("Removed attribute still present in SessionMap ?", abstractMap.get("KEY"));
+ assertNull("Removed attribute still present in HttpSessionMock ?", httpSessionMock.getAttribute("KEY"));
+ sessionMock.verify();
+ }
+
+ @Override
protected void setUp() throws Exception {
sessionMock = new Mock(HttpSession.class);
sessionMock.matchAndReturn("getId", "1");
@@ -235,16 +295,19 @@
private static final long serialVersionUID = 8783604360786273764L;
- private Map<String, Object> map = new HashMap<>();
+ private final Map<String, Object> map;
public MockSessionMap(HttpServletRequest request) {
super(request);
+ this.map = new HashMap<>();
}
+ @Override
public Object get(Object key) {
return map.get(key);
}
+ @Override
public Object put(String key, Object value) {
Object originalValue = super.put(key, value);
map.put(key, value); //put the value into our map after putting it in the superclass map to avoid polluting the get call.
@@ -252,6 +315,7 @@
return originalValue;
}
+ @Override
public void clear() {
super.clear();
map.clear();
