Apache/struts
1
0
Fork 0
mirror of https://github.com/apache/struts.git synced 2026-08-11 01:27:14 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #688 from JCgH4164838Gh792C124B5/localS2_62_ExecWaitCleanup

Browse Source 
[WW-5312] Attempt to fix ExecuteAndWaitInterceptor inconsistent processing
This commit is contained in:
Lukasz Lenart
2023-05-28 09:07:46 +02:00
committed by GitHub
parent 88c847e535 dd01a47ddb
commit c92f542180
4 changed files with 147 additions and 56 deletions
Download Patch File Download Diff File Expand all files Collapse all files
core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java
+25 -11
View File
@@ -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);
}
}
}
core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java
+33 -20
View File
@@ -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();
}
}
}
core/src/main/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcess.java
+4 -4
View File
@@ -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);
core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java
+85 -21
View File
@@ -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();