This is an automated email from the ASF dual-hosted git repository.
lukaszlenart pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/struts.git
The following commit(s) were added to refs/heads/master by this push:
new dd01a47dd Update: - Improve ExecuteAndWaitInterceptor state behaviour,
added debug and trace logging. - Ensure StrutsBackgroundProcess thread done
state always set on completion. - Fix SessionMap processing issue caused by
divergence of method signatures, which can result in the ancestor methods
getting called (bypassing actual session processing). - Update SessionMapTest
to correspond to changes, add tests to try and detect some (put/remove) method
signature behaviour changes.
new c92f54218 Merge pull request #688 from
JCgH4164838Gh792C124B5/localS2_62_ExecWaitCleanup
dd01a47dd is described below
commit dd01a47ddb61fce51b5ef1015a1ec4c2db33da89
Author: JCgH4164838Gh792C124B5
<43964333+jcgh4164838gh792c12...@users.noreply.github.com>
AuthorDate: Sat May 20 18:05:04 2023 -0400
Update:
- Improve ExecuteAndWaitInterceptor state behaviour, added debug and trace
logging.
- Ensure StrutsBackgroundProcess thread done state always set on
completion.
- Fix SessionMap processing issue caused by divergence of method
signatures, which can result in the ancestor methods getting called
(bypassing actual session processing).
- Update SessionMapTest to correspond to changes, add tests to try and
detect some (put/remove) method signature behaviour changes.
---
.../org/apache/struts2/dispatcher/SessionMap.java | 36 ++++---
.../interceptor/ExecuteAndWaitInterceptor.java | 53 +++++++----
.../interceptor/exec/StrutsBackgroundProcess.java | 8 +-
.../apache/struts2/dispatcher/SessionMapTest.java | 106 +++++++++++++++++----
4 files changed, 147 insertions(+), 56 deletions(-)
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 dac8404ea..8fe5777ea 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 @@ public class SessionMap extends AbstractMap<String, Object>
implements Serializa
/**
- * 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 @@ public class SessionMap extends AbstractMap<String, Object>
implements Serializa
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 @@ public class SessionMap extends AbstractMap<String, Object>
implements Serializa
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 @@ public class SessionMap extends AbstractMap<String,
Object> implements Serializa
/**
* 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 @@ public class SessionMap extends AbstractMap<String, Object>
implements Serializa
}
}
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 @@ public class SessionMap extends AbstractMap<String,
Object> implements Serializa
/**
* 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 @@ public class SessionMap extends AbstractMap<String, Object>
implements Serializa
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 @@ public class SessionMap extends AbstractMap<String,
Object> implements Serializa
/**
* 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 7a022c9cb..8d487661d 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 @@ public class ExecuteAndWaitInterceptor extends
MethodFilterInterceptor {
/* (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 @@ public class ExecuteAndWaitInterceptor extends
MethodFilterInterceptor {
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 @@ public class ExecuteAndWaitInterceptor extends
MethodFilterInterceptor {
}
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 @@ public class ExecuteAndWaitInterceptor extends
MethodFilterInterceptor {
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 @@ public class ExecuteAndWaitInterceptor extends
MethodFilterInterceptor {
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 @@ public class ExecuteAndWaitInterceptor extends
MethodFilterInterceptor {
@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 8f563912b..42237262f 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 @@ public class StrutsBackgroundProcess implements
BackgroundProcess, Serializable
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 @@ public class StrutsBackgroundProcess implements
BackgroundProcess, Serializable
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 8c292cdd5..6dc597ebd 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.Constraint;
import com.mockobjects.constraint.IsAnything;
import com.mockobjects.constraint.IsEqual;
import com.mockobjects.dynamic.Mock;
+import java.util.AbstractMap;
/**
@@ -47,24 +48,18 @@ public class SessionMapTest extends TestCase {
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 @@ public class SessionMapTest extends TestCase {
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 @@ public class SessionMapTest extends TestCase {
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 @@ public class SessionMapTest extends TestCase {
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 @@ public class SessionMapTest extends TestCase {
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 @@ public class SessionMapTest extends TestCase {
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 @@ public class SessionMapTest extends TestCase {
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 @@ public class SessionMapTest extends TestCase {
return originalValue;
}
+ @Override
public void clear() {
super.clear();
map.clear();
