tapestry-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From hls...@apache.org
Subject [3/3] git commit: TAP5-2049: Use a lock when reading/updating HttpSession attributes
Date Fri, 11 Jan 2013 17:26:50 GMT
Updated Branches:
  refs/heads/master d543df368 -> bebe2d141


TAP5-2049: Use a lock when reading/updating HttpSession attributes


Project: http://git-wip-us.apache.org/repos/asf/tapestry-5/repo
Commit: http://git-wip-us.apache.org/repos/asf/tapestry-5/commit/bebe2d14
Tree: http://git-wip-us.apache.org/repos/asf/tapestry-5/tree/bebe2d14
Diff: http://git-wip-us.apache.org/repos/asf/tapestry-5/diff/bebe2d14

Branch: refs/heads/master
Commit: bebe2d1417fcfa0b32bd8aa8abd246b3e0c1504c
Parents: 33d23b6
Author: Howard M. Lewis Ship <hlship@apache.org>
Authored: Fri Jan 11 12:26:36 2013 -0500
Committer: Howard M. Lewis Ship <hlship@apache.org>
Committed: Fri Jan 11 12:26:36 2013 -0500

----------------------------------------------------------------------
 .../internal/services/ClusteredSessionImpl.java    |   26 +++-
 .../tapestry5/internal/services/RequestImpl.java   |    4 +-
 .../tapestry5/internal/services/SessionImpl.java   |   65 ++++++++-
 .../services/TapestrySessionFactoryImpl.java       |   15 ++-
 .../internal/services/RequestImplTest.java         |   49 +------
 .../internal/services/SessionImplTest.java         |  108 ++++++++++++--
 6 files changed, 191 insertions(+), 76 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/bebe2d14/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ClusteredSessionImpl.java
----------------------------------------------------------------------
diff --git a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ClusteredSessionImpl.java
b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ClusteredSessionImpl.java
index 0e52e5c..5e8339b 100644
--- a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ClusteredSessionImpl.java
+++ b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ClusteredSessionImpl.java
@@ -1,4 +1,4 @@
-//  Copyright 2011 The Apache Software Foundation
+//  Copyright 2011, 2013 The Apache Software Foundation
 //
 //  Licensed under the Apache License, Version 2.0 (the "License");
 //  you may not use this file except in compliance with the License.
@@ -15,6 +15,7 @@
 package org.apache.tapestry5.internal.services;
 
 import org.apache.tapestry5.ioc.internal.util.CollectionFactory;
+import org.apache.tapestry5.ioc.services.PerthreadManager;
 import org.apache.tapestry5.services.SessionPersistedObjectAnalyzer;
 
 import javax.servlet.http.HttpServletRequest;
@@ -22,9 +23,12 @@ import javax.servlet.http.HttpSession;
 import java.util.Map;
 
 /**
- * A thin wrapper around {@link javax.servlet.http.HttpSession}.
+ * A thin wrapper around {@link javax.servlet.http.HttpSession} that supports re-storing
of mutable
+ * session attributes at the end of the request (see {@link #restoreDirtyObjects()}). This
is only
+ * used when {@linkplain org.apache.tapestry5.SymbolConstants#CLUSTERED_SESSIONS clustering}.
  *
  * @since 5.3
+ * @see SessionPersistedObjectAnalyzer
  */
 public class ClusteredSessionImpl extends SessionImpl
 {
@@ -40,9 +44,10 @@ public class ClusteredSessionImpl extends SessionImpl
     public ClusteredSessionImpl(
             HttpServletRequest request,
             HttpSession session,
+            PerthreadManager perthreadManager,
             SessionPersistedObjectAnalyzer analyzer)
     {
-        super(request, session);
+        super(request, session, perthreadManager);
         this.analyzer = analyzer;
     }
 
@@ -72,9 +77,15 @@ public class ClusteredSessionImpl extends SessionImpl
 
     public void restoreDirtyObjects()
     {
-        if (isInvalidated()) return;
+        if (isInvalidated())
+        {
+            return;
+        }
 
-        if (sessionAttributeCache.isEmpty()) return;
+        if (sessionAttributeCache.isEmpty())
+        {
+            return;
+        }
 
         for (Map.Entry<String, Object> entry : sessionAttributeCache.entrySet())
         {
@@ -82,7 +93,10 @@ public class ClusteredSessionImpl extends SessionImpl
 
             Object attributeValue = entry.getValue();
 
-            if (attributeValue == null) continue;
+            if (attributeValue == null)
+            {
+                continue;
+            }
 
             if (analyzer.checkAndResetDirtyState(attributeValue))
             {

http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/bebe2d14/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/RequestImpl.java
----------------------------------------------------------------------
diff --git a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/RequestImpl.java
b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/RequestImpl.java
index 45a469e..6216f5a 100644
--- a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/RequestImpl.java
+++ b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/RequestImpl.java
@@ -1,4 +1,4 @@
-// Copyright 2006, 2007, 2008, 2010, 2011, 2012 The Apache Software Foundation
+// Copyright 2006-2013 The Apache Software Foundation
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -42,7 +42,7 @@ public class RequestImpl implements Request
 
     private boolean encodingSet;
 
-    Session session;
+    private Session session;
 
     public RequestImpl(
             HttpServletRequest request,

http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/bebe2d14/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/SessionImpl.java
----------------------------------------------------------------------
diff --git a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/SessionImpl.java
b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/SessionImpl.java
index 89cf837..3a93045 100644
--- a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/SessionImpl.java
+++ b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/SessionImpl.java
@@ -1,4 +1,4 @@
-// Copyright 2006, 2007, 2008, 2009, 2011 The Apache Software Foundation
+// Copyright 2006-2013 The Apache Software Foundation
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -16,15 +16,15 @@ package org.apache.tapestry5.internal.services;
 
 import org.apache.tapestry5.ioc.internal.util.CollectionFactory;
 import org.apache.tapestry5.ioc.internal.util.InternalUtils;
+import org.apache.tapestry5.ioc.services.PerthreadManager;
 import org.apache.tapestry5.services.Session;
-import org.apache.tapestry5.services.SessionPersistedObjectAnalyzer;
 
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpSession;
 import java.util.Collections;
 import java.util.Enumeration;
 import java.util.List;
-import java.util.Map;
+import java.util.concurrent.locks.ReentrantLock;
 
 /**
  * A thin wrapper around {@link HttpSession}.
@@ -32,33 +32,90 @@ import java.util.Map;
 public class SessionImpl implements Session
 {
     private final HttpServletRequest request;
+
     private final HttpSession session;
 
     private boolean invalidated = false;
 
-    public SessionImpl(HttpServletRequest request, HttpSession session)
+    private final PerthreadManager perthreadManager;
+
+    private final ReentrantLock lock;
+
+    final static String LOCK_KEY = "org.apache.tapestry5.SessionLock";
+
+    public SessionImpl(HttpServletRequest request, HttpSession session, PerthreadManager
perthreadManager)
     {
         this.request = request;
         this.session = session;
+        this.perthreadManager = perthreadManager;
+
+        lock = findOrCreateLock();
+    }
+
+    private ReentrantLock findOrCreateLock()
+    {
+
+        // Yes, this itself is a problem as multiple threads may attempt to create the HttpSession
simultaneously.
+        // I suspect that is quite rare however.
+
+        ReentrantLock result = (ReentrantLock) session.getAttribute(LOCK_KEY);
+
+        if (result == null)
+        {
+            result = new ReentrantLock();
+            session.setAttribute(LOCK_KEY, result);
+        }
+
+        return result;
+    }
+
+    /**
+     * Gains the exclusive lock needed to perform any access to attributes inside the session.
The lock is acquired
+     * on any read or write access, and is held until the request completes.
+     */
+    private void lock()
+    {
+        if (!lock.isLocked())
+        {
+            // The HttpSession may be shared across threads, but the lock (almost) certainly
is.
+            lock.lock();
+
+            perthreadManager.addThreadCleanupCallback(new Runnable()
+            {
+                @Override
+                public void run()
+                {
+                    lock.unlock();
+                }
+            });
+        }
     }
 
     public Object getAttribute(String name)
     {
+        lock();
+
         return session.getAttribute(name);
     }
 
     public List<String> getAttributeNames()
     {
+        lock();
+
         return InternalUtils.toList(session.getAttributeNames());
     }
 
     public void setAttribute(String name, Object value)
     {
+        lock();
+
         session.setAttribute(name, value);
     }
 
     public List<String> getAttributeNames(String prefix)
     {
+        lock();
+
         List<String> result = CollectionFactory.newList();
 
         Enumeration e = session.getAttributeNames();

http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/bebe2d14/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/TapestrySessionFactoryImpl.java
----------------------------------------------------------------------
diff --git a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/TapestrySessionFactoryImpl.java
b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/TapestrySessionFactoryImpl.java
index caec281..6a38372 100644
--- a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/TapestrySessionFactoryImpl.java
+++ b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/TapestrySessionFactoryImpl.java
@@ -1,4 +1,4 @@
-//  Copyright 2011 The Apache Software Foundation
+//  Copyright 2011, 2013 The Apache Software Foundation
 //
 //  Licensed under the Apache License, Version 2.0 (the "License");
 //  you may not use this file except in compliance with the License.
@@ -16,6 +16,7 @@ package org.apache.tapestry5.internal.services;
 
 import org.apache.tapestry5.SymbolConstants;
 import org.apache.tapestry5.ioc.annotations.Symbol;
+import org.apache.tapestry5.ioc.services.PerthreadManager;
 import org.apache.tapestry5.services.Session;
 import org.apache.tapestry5.services.SessionPersistedObjectAnalyzer;
 
@@ -25,18 +26,24 @@ import javax.servlet.http.HttpSession;
 public class TapestrySessionFactoryImpl implements TapestrySessionFactory
 {
     private boolean clustered;
+
     private final SessionPersistedObjectAnalyzer analyzer;
+
     private final HttpServletRequest request;
 
+    private final PerthreadManager perthreadManager;
+
     public TapestrySessionFactoryImpl(
             @Symbol(SymbolConstants.CLUSTERED_SESSIONS)
             boolean clustered,
             SessionPersistedObjectAnalyzer analyzer,
-            HttpServletRequest request)
+            HttpServletRequest request,
+            PerthreadManager perthreadManager)
     {
         this.clustered = clustered;
         this.analyzer = analyzer;
         this.request = request;
+        this.perthreadManager = perthreadManager;
     }
 
     public Session getSession(boolean create)
@@ -50,9 +57,9 @@ public class TapestrySessionFactoryImpl implements TapestrySessionFactory
 
         if (clustered)
         {
-            return new ClusteredSessionImpl(request, httpSession, analyzer);
+            return new ClusteredSessionImpl(request, httpSession, perthreadManager, analyzer);
         }
 
-        return new SessionImpl(request, httpSession);
+        return new SessionImpl(request, httpSession, perthreadManager);
     }
 }

http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/bebe2d14/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/RequestImplTest.java
----------------------------------------------------------------------
diff --git a/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/RequestImplTest.java
b/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/RequestImplTest.java
index a13b67b..ed5367d 100644
--- a/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/RequestImplTest.java
+++ b/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/RequestImplTest.java
@@ -1,4 +1,4 @@
-// Copyright 2006, 2007, 2008, 2009, 2011, 2012 The Apache Software Foundation
+// Copyright 2006-2013 The Apache Software Foundation
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -29,6 +29,7 @@ public class RequestImplTest extends InternalBaseTestCase
 {
     public static final String CHARSET = "UTF-8";
 
+
     @Test
     public void get_session_doesnt_exist()
     {
@@ -52,17 +53,15 @@ public class RequestImplTest extends InternalBaseTestCase
         HttpServletRequest sr = mockHttpServletRequest();
         HttpSession ss = mockHttpSession();
         TapestrySessionFactory sf = newMock(TapestrySessionFactory.class);
+        Session session = mockSession();
 
-        expect(sf.getSession(true)).andReturn(new SessionImpl(sr, ss));
-
-        train_getAttribute(ss, "foo", "bar");
+        expect(sf.getSession(true)).andReturn(session);
 
         replay();
 
         Request request = new RequestImpl(sr, CHARSET, sf);
-        Session session = request.getSession(true);
 
-        assertEquals(session.getAttribute("foo"), "bar");
+        assertSame(request.getSession(true), session);
 
         verify();
     }
@@ -196,44 +195,6 @@ public class RequestImplTest extends InternalBaseTestCase
         verify();
     }
 
-    @Test
-    public void get_session_returns_null_if_invalid()
-    {
-        HttpServletRequest sr = mockHttpServletRequest();
-        HttpSession hsession1 = mockHttpSession();
-        HttpSession hsession2 = mockHttpSession();
-
-        TapestrySessionFactory sf = newMock(TapestrySessionFactory.class);
-
-        expect(sf.getSession(true)).andReturn(new SessionImpl(sr, hsession1));
-
-        replay();
-
-        Request request = new RequestImpl(sr, CHARSET, sf);
-
-        Session session1 = request.getSession(true);
-
-        verify();
-
-        hsession1.invalidate();
-
-        expect(sr.getSession(false)).andReturn(null);
-
-        SessionImpl session = new SessionImpl(sr, hsession2);
-        expect(sf.getSession(true)).andReturn(session);
-        expect(sf.getSession(true)).andReturn(session);
-
-        replay();
-
-        session1.invalidate();
-
-        Session session2 = request.getSession(true);
-
-        assertNotSame(session2, session1);
-        assertSame(request.getSession(true), session2);
-
-        verify();
-    }
 
     protected final void train_getPathInfo(HttpServletRequest request, String pathInfo)
     {

http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/bebe2d14/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/SessionImplTest.java
----------------------------------------------------------------------
diff --git a/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/SessionImplTest.java
b/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/SessionImplTest.java
index d8b012a..0014abc 100644
--- a/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/SessionImplTest.java
+++ b/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/SessionImplTest.java
@@ -1,4 +1,4 @@
-// Copyright 2006, 2007, 2008, 2009, 2011 The Apache Software Foundation
+// Copyright 2006-2013 The Apache Software Foundation
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -14,34 +14,99 @@
 
 package org.apache.tapestry5.internal.services;
 
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.Enumeration;
-
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpSession;
-
 import org.apache.tapestry5.internal.test.InternalBaseTestCase;
+import org.apache.tapestry5.internal.util.Holder;
+import org.apache.tapestry5.ioc.services.PerthreadManager;
+import org.apache.tapestry5.services.Request;
 import org.apache.tapestry5.services.Session;
 import org.apache.tapestry5.services.SessionPersistedObjectAnalyzer;
+import org.easymock.EasyMock;
+import org.easymock.IAnswer;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeClass;
 import org.testng.annotations.Test;
 
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpSession;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Enumeration;
+import java.util.concurrent.locks.ReentrantLock;
+
 public class SessionImplTest extends InternalBaseTestCase
 {
+    private PerthreadManager perThreadManager;
+
+    private final ReentrantLock lock = new ReentrantLock();
+
+    @BeforeClass
+    public void setup()
+    {
+        perThreadManager = getService(PerthreadManager.class);
+    }
+
+    @AfterMethod
+    public void releaseLock()
+    {
+        perThreadManager.cleanup();
+
+        assertFalse(lock.isLocked());
+    }
+
+    void trainForLock(HttpSession session)
+    {
+        expect(session.getAttribute(SessionImpl.LOCK_KEY)).andReturn(lock);
+    }
+
+    @Test
+    public void will_create_lock_if_not_present()
+    {
+        HttpSession hs = mockHttpSession();
+
+        final Holder<ReentrantLock> holder = Holder.create();
+
+        expect(hs.getAttribute(SessionImpl.LOCK_KEY)).andReturn(null);
+
+        hs.setAttribute(EasyMock.eq(SessionImpl.LOCK_KEY), EasyMock.isA(ReentrantLock.class));
+        EasyMock.expectLastCall().andStubAnswer(new IAnswer<Object>()
+        {
+            public Object answer() throws Throwable
+            {
+                ReentrantLock lock = (ReentrantLock) EasyMock.getCurrentArguments()[1];
+
+                holder.put(lock);
+
+                return null;
+            }
+        });
+
+        replay();
+
+        new SessionImpl(null, hs, perThreadManager);
+
+        assertFalse(holder.get().isLocked());
+
+        verify();
+    }
+
     @Test
     public void get_attribute_names()
     {
         Enumeration e = Collections.enumeration(Arrays.asList("fred", "barney"));
         HttpSession hs = mockHttpSession();
 
+        trainForLock(hs);
+
         expect(hs.getAttributeNames()).andReturn(e);
 
         replay();
 
-        Session session = new SessionImpl(null, hs);
+        Session session = new SessionImpl(null, hs, perThreadManager);
 
         assertEquals(session.getAttributeNames(), Arrays.asList("barney", "fred"));
 
+        assertTrue(lock.isLocked());
+
         verify();
     }
 
@@ -51,14 +116,18 @@ public class SessionImplTest extends InternalBaseTestCase
         Enumeration e = Collections.enumeration(Arrays.asList("fred", "barney", "fanny"));
         HttpSession hs = mockHttpSession();
 
+        trainForLock(hs);
+
         expect(hs.getAttributeNames()).andReturn(e);
 
         replay();
 
-        Session session = new SessionImpl(null, hs);
+        Session session = new SessionImpl(null, hs, perThreadManager);
 
         assertEquals(session.getAttributeNames("f"), Arrays.asList("fanny", "fred"));
 
+        assertTrue(lock.isLocked());
+
         verify();
     }
 
@@ -67,11 +136,13 @@ public class SessionImplTest extends InternalBaseTestCase
     {
         HttpSession hs = mockHttpSession();
 
+        trainForLock(hs);
+
         hs.invalidate();
 
         replay();
 
-        Session session = new SessionImpl(null, hs);
+        Session session = new SessionImpl(null, hs, perThreadManager);
 
         session.invalidate();
 
@@ -82,14 +153,14 @@ public class SessionImplTest extends InternalBaseTestCase
     public void http_session_invalidate()
     {
         HttpSession hs = mockHttpSession();
-
         HttpServletRequest hsr = mockHttpServletRequest();
 
         train_getSession(hsr, false, hs);
+        trainForLock(hs);
 
         replay();
 
-        Session session = new SessionImpl(hsr, hs);
+        Session session = new SessionImpl(hsr, hs, perThreadManager);
 
         assertFalse(session.isInvalidated());
 
@@ -116,12 +187,13 @@ public class SessionImplTest extends InternalBaseTestCase
     {
         HttpSession hs = mockHttpSession();
         int seconds = 999;
+        trainForLock(hs);
 
         hs.setMaxInactiveInterval(seconds);
 
         replay();
 
-        Session session = new SessionImpl(null, hs);
+        Session session = new SessionImpl(null, hs, perThreadManager);
 
         session.setMaxInactiveInterval(seconds);
 
@@ -132,13 +204,16 @@ public class SessionImplTest extends InternalBaseTestCase
     public void get_max_inactive()
     {
         HttpSession hs = mockHttpSession();
+
+        trainForLock(hs);
+
         int seconds = 999;
 
         expect(hs.getMaxInactiveInterval()).andReturn(seconds);
 
         replay();
 
-        Session session = new SessionImpl(null, hs);
+        Session session = new SessionImpl(null, hs, perThreadManager);
 
         assertEquals(session.getMaxInactiveInterval(), seconds);
 
@@ -153,11 +228,12 @@ public class SessionImplTest extends InternalBaseTestCase
         SessionPersistedObjectAnalyzer analyzer = newMock(SessionPersistedObjectAnalyzer.class);
         Object dirty = new Object();
 
+        trainForLock(hs);
         train_getAttribute(hs, "dirty", dirty);
 
         replay();
 
-        Session session = new ClusteredSessionImpl(hsr, hs, analyzer);
+        Session session = new ClusteredSessionImpl(hsr, hs, perThreadManager, analyzer);
 
         assertSame(session.getAttribute("dirty"), dirty);
 


Mime
View raw message