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/45dc6744 Tree: http://git-wip-us.apache.org/repos/asf/geode/tree/45dc6744 Diff: http://git-wip-us.apache.org/repos/asf/geode/diff/45dc6744 Branch: refs/heads/feature/GEM-1299 Commit: 45dc6744154a08986e833852ef743af5d8bf19ba Parents: 47d8c82 Author: Dan Smith <upthewatersp...@apache.org> Authored: Fri Apr 21 11:36:24 2017 -0700 Committer: Dan Smith <upthewatersp...@apache.org> Committed: Fri Apr 21 16:23:24 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/45dc6744/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/45dc6744/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/45dc6744/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/45dc6744/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/45dc6744/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/45dc6744/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/45dc6744/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