GEODE-2808 - Fixing lock ordering issues in DeltaSession Region expiration of sessions and explicit expiration of sessions had lock ordering issues. Fixing the code so that expiration goes through the region entry lock first, before getting the lock on StandardSession.
Adding a workaround for the fact that liferay calls removeAttribute from within session expiration by ignoreing remoteAttribute calls during expiration. This closes #472 Project: http://git-wip-us.apache.org/repos/asf/geode/repo Commit: http://git-wip-us.apache.org/repos/asf/geode/commit/80a95f60 Tree: http://git-wip-us.apache.org/repos/asf/geode/tree/80a95f60 Diff: http://git-wip-us.apache.org/repos/asf/geode/diff/80a95f60 Branch: refs/heads/feature/GEM-1299 Commit: 80a95f6047ad64d165744f5ae4088b409484d50f Parents: 5a8d03d Author: Dan Smith <[email protected]> Authored: Fri Apr 21 11:36:24 2017 -0700 Committer: zhouxh <[email protected]> Committed: Wed Apr 26 23:23:48 2017 -0700 ---------------------------------------------------------------------- .../modules/session/catalina/DeltaSession7.java | 14 +++++++- .../modules/session/catalina/DeltaSession8.java | 14 +++++++- .../session/TestSessionsTomcat8Base.java | 34 ++++++++++++++++++++ .../modules/session/catalina/DeltaSession.java | 14 +++++++- .../geode/modules/session/CommandServlet.java | 4 +++ .../geode/modules/session/QueryCommand.java | 2 ++ .../geode/modules/session/TestSessionsBase.java | 34 ++++++++++++++++++++ 7 files changed, 113 insertions(+), 3 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/geode/blob/80a95f60/extensions/geode-modules-tomcat7/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession7.java ---------------------------------------------------------------------- diff --git a/extensions/geode-modules-tomcat7/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession7.java b/extensions/geode-modules-tomcat7/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession7.java index 204ff5e..d7f30bd 100644 --- a/extensions/geode-modules-tomcat7/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession7.java +++ b/extensions/geode-modules-tomcat7/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession7.java @@ -263,6 +263,9 @@ public class DeltaSession7 extends StandardSession public void removeAttribute(String name, boolean notify) { checkBackingCacheAvailable(); + if (expired) { + return; + } synchronized (this.changeLock) { // Remove the attribute locally super.removeAttribute(name, true); @@ -322,7 +325,7 @@ public class DeltaSession7 extends StandardSession setExpired(true); // Do expire processing - expire(); + super.expire(true); // Update statistics if (manager != null) { @@ -330,6 +333,15 @@ public class DeltaSession7 extends StandardSession } } + @Override + public void expire(boolean notify) { + if (notify) { + getOperatingRegion().destroy(this.getId(), this); + } else { + super.expire(false); + } + } + public void setMaxInactiveInterval(int interval) { super.setMaxInactiveInterval(interval); } http://git-wip-us.apache.org/repos/asf/geode/blob/80a95f60/extensions/geode-modules-tomcat8/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession8.java ---------------------------------------------------------------------- diff --git a/extensions/geode-modules-tomcat8/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession8.java b/extensions/geode-modules-tomcat8/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession8.java index b5e7d0c..f69382a 100644 --- a/extensions/geode-modules-tomcat8/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession8.java +++ b/extensions/geode-modules-tomcat8/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession8.java @@ -258,6 +258,9 @@ public class DeltaSession8 extends StandardSession public void removeAttribute(String name, boolean notify) { checkBackingCacheAvailable(); + if (expired) { + return; + } synchronized (this.changeLock) { // Remove the attribute locally super.removeAttribute(name, true); @@ -317,7 +320,7 @@ public class DeltaSession8 extends StandardSession setExpired(true); // Do expire processing - expire(); + super.expire(true); // Update statistics if (manager != null) { @@ -325,6 +328,15 @@ public class DeltaSession8 extends StandardSession } } + @Override + public void expire(boolean notify) { + if (notify) { + getOperatingRegion().destroy(this.getId(), this); + } else { + super.expire(false); + } + } + public void setMaxInactiveInterval(int interval) { super.setMaxInactiveInterval(interval); } http://git-wip-us.apache.org/repos/asf/geode/blob/80a95f60/extensions/geode-modules-tomcat8/src/test/java/org/apache/geode/modules/session/TestSessionsTomcat8Base.java ---------------------------------------------------------------------- diff --git a/extensions/geode-modules-tomcat8/src/test/java/org/apache/geode/modules/session/TestSessionsTomcat8Base.java b/extensions/geode-modules-tomcat8/src/test/java/org/apache/geode/modules/session/TestSessionsTomcat8Base.java index 15b3874..1dc1d8b 100644 --- a/extensions/geode-modules-tomcat8/src/test/java/org/apache/geode/modules/session/TestSessionsTomcat8Base.java +++ b/extensions/geode-modules-tomcat8/src/test/java/org/apache/geode/modules/session/TestSessionsTomcat8Base.java @@ -233,6 +233,40 @@ public abstract class TestSessionsTomcat8Base extends JUnit4DistributedTestCase } /** + * Test expiration of a session by the tomcat container, rather than gemfire expiration + */ + @Test + public void testSessionExpirationByContainer() throws Exception { + + String key = "value_testSessionExpiration1"; + String value = "Foo"; + + WebConversation wc = new WebConversation(); + WebRequest req = new GetMethodWebRequest(String.format("http://localhost:%d/test";, port)); + + // Set an attribute + req.setParameter("cmd", QueryCommand.SET.name()); + req.setParameter("param", key); + req.setParameter("value", value); + WebResponse response = wc.getResponse(req); + + // Set the session timeout of this one session. + req.setParameter("cmd", QueryCommand.SET_MAX_INACTIVE.name()); + req.setParameter("value", "1"); + response = wc.getResponse(req); + + // Wait until the session should expire + Thread.sleep(2000); + + // Do a request, which should cause the session to be expired + req.setParameter("cmd", QueryCommand.GET.name()); + req.setParameter("param", key); + response = wc.getResponse(req); + + assertEquals("", response.getText()); + } + + /** * Test that removing a session attribute also removes it from the region */ @Test http://git-wip-us.apache.org/repos/asf/geode/blob/80a95f60/extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession.java ---------------------------------------------------------------------- diff --git a/extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession.java b/extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession.java index bc421a5..ac612da 100644 --- a/extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession.java +++ b/extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession.java @@ -266,6 +266,9 @@ public class DeltaSession extends StandardSession public void removeAttribute(String name, boolean notify) { checkBackingCacheAvailable(); + if (expired) { + return; + } synchronized (this.changeLock) { // Remove the attribute locally super.removeAttribute(name, true); @@ -325,7 +328,7 @@ public class DeltaSession extends StandardSession setExpired(true); // Do expire processing - expire(); + super.expire(true); // Update statistics if (manager != null) { @@ -333,6 +336,15 @@ public class DeltaSession extends StandardSession } } + @Override + public void expire(boolean notify) { + if (notify) { + getOperatingRegion().destroy(this.getId(), this); + } else { + super.expire(false); + } + } + public void setMaxInactiveInterval(int interval) { super.setMaxInactiveInterval(interval); } http://git-wip-us.apache.org/repos/asf/geode/blob/80a95f60/extensions/geode-modules/src/test/java/org/apache/geode/modules/session/CommandServlet.java ---------------------------------------------------------------------- diff --git a/extensions/geode-modules/src/test/java/org/apache/geode/modules/session/CommandServlet.java b/extensions/geode-modules/src/test/java/org/apache/geode/modules/session/CommandServlet.java index 3fede62..a04194b 100644 --- a/extensions/geode-modules/src/test/java/org/apache/geode/modules/session/CommandServlet.java +++ b/extensions/geode-modules/src/test/java/org/apache/geode/modules/session/CommandServlet.java @@ -60,6 +60,10 @@ public class CommandServlet extends HttpServlet { session = request.getSession(); session.setAttribute(param, value); break; + case SET_MAX_INACTIVE: + session = request.getSession(); + session.setMaxInactiveInterval(Integer.valueOf(value)); + break; case GET: session = request.getSession(); String val = (String) session.getAttribute(param); http://git-wip-us.apache.org/repos/asf/geode/blob/80a95f60/extensions/geode-modules/src/test/java/org/apache/geode/modules/session/QueryCommand.java ---------------------------------------------------------------------- diff --git a/extensions/geode-modules/src/test/java/org/apache/geode/modules/session/QueryCommand.java b/extensions/geode-modules/src/test/java/org/apache/geode/modules/session/QueryCommand.java index 2b89e68..622866e 100644 --- a/extensions/geode-modules/src/test/java/org/apache/geode/modules/session/QueryCommand.java +++ b/extensions/geode-modules/src/test/java/org/apache/geode/modules/session/QueryCommand.java @@ -21,6 +21,8 @@ public enum QueryCommand { SET, + SET_MAX_INACTIVE, + GET, INVALIDATE, http://git-wip-us.apache.org/repos/asf/geode/blob/80a95f60/extensions/geode-modules/src/test/java/org/apache/geode/modules/session/TestSessionsBase.java ---------------------------------------------------------------------- diff --git a/extensions/geode-modules/src/test/java/org/apache/geode/modules/session/TestSessionsBase.java b/extensions/geode-modules/src/test/java/org/apache/geode/modules/session/TestSessionsBase.java index d7674dd..a6bec6c 100644 --- a/extensions/geode-modules/src/test/java/org/apache/geode/modules/session/TestSessionsBase.java +++ b/extensions/geode-modules/src/test/java/org/apache/geode/modules/session/TestSessionsBase.java @@ -267,6 +267,40 @@ public abstract class TestSessionsBase { } /** + * Test expiration of a session by the tomcat container, rather than gemfire expiration + */ + @Test + public void testSessionExpirationByContainer() throws Exception { + + String key = "value_testSessionExpiration1"; + String value = "Foo"; + + WebConversation wc = new WebConversation(); + WebRequest req = new GetMethodWebRequest(String.format("http://localhost:%d/test";, port)); + + // Set an attribute + req.setParameter("cmd", QueryCommand.SET.name()); + req.setParameter("param", key); + req.setParameter("value", value); + WebResponse response = wc.getResponse(req); + + // Set the session timeout of this one session. + req.setParameter("cmd", QueryCommand.SET_MAX_INACTIVE.name()); + req.setParameter("value", "1"); + response = wc.getResponse(req); + + // Wait until the session should expire + Thread.sleep(2000); + + // Do a request, which should cause the session to be expired + req.setParameter("cmd", QueryCommand.GET.name()); + req.setParameter("param", key); + response = wc.getResponse(req); + + assertEquals("", response.getText()); + } + + /** * Test that removing a session attribute also removes it from the region */ @Test
