TAP5-2049: Use a lock when reading/updating HttpSession attributes - move the lock from the inside the session to a WeakHashMap keyed on HttpSession instance - allow shared read lock for reading attribute names - upgrade to write lock for getAttribute() or setAttribute()
Project: http://git-wip-us.apache.org/repos/asf/tapestry-5/repo Commit: http://git-wip-us.apache.org/repos/asf/tapestry-5/commit/680ffc3a Tree: http://git-wip-us.apache.org/repos/asf/tapestry-5/tree/680ffc3a Diff: http://git-wip-us.apache.org/repos/asf/tapestry-5/diff/680ffc3a Branch: refs/heads/master Commit: 680ffc3a3b8216dc73a4de61c6c332cff9836147 Parents: fb3fca8 Author: Howard M. Lewis Ship <[email protected]> Authored: Thu Jan 17 17:42:38 2013 +0000 Committer: Howard M. Lewis Ship <[email protected]> Committed: Thu Jan 17 17:42:38 2013 +0000 ---------------------------------------------------------------------- .../internal/services/ClusteredSessionImpl.java | 9 +- .../tapestry5/internal/services/SessionImpl.java | 59 +-------- .../tapestry5/internal/services/SessionLock.java | 36 +++++ .../services/TapestrySessionFactoryImpl.java | 102 ++++++++++++++- .../internal/services/SessionImplTest.java | 99 +++----------- 5 files changed, 167 insertions(+), 138 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/680ffc3a/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 5e8339b..3dccb17 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 @@ -41,13 +41,12 @@ public class ClusteredSessionImpl extends SessionImpl */ private final Map<String, Object> sessionAttributeCache = CollectionFactory.newMap(); - public ClusteredSessionImpl( - HttpServletRequest request, + public ClusteredSessionImpl(HttpServletRequest request, HttpSession session, - PerthreadManager perthreadManager, - SessionPersistedObjectAnalyzer analyzer) + SessionLock lock, SessionPersistedObjectAnalyzer analyzer) { - super(request, session, perthreadManager); + super(request, session, lock); + this.analyzer = analyzer; } http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/680ffc3a/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 33b38d9..7829285 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 @@ -24,7 +24,6 @@ import javax.servlet.http.HttpSession; import java.util.Collections; import java.util.Enumeration; import java.util.List; -import java.util.concurrent.locks.ReentrantLock; /** * A thin wrapper around {@link HttpSession}. @@ -37,83 +36,39 @@ public class SessionImpl implements Session private boolean invalidated = false; - private final PerthreadManager perthreadManager; + private final SessionLock lock; - private final ReentrantLock lock; - - final static String LOCK_KEY = "org.apache.tapestry5.SessionLock"; - - public SessionImpl(HttpServletRequest request, HttpSession session, PerthreadManager perthreadManager) + public SessionImpl(HttpServletRequest request, HttpSession session, SessionLock lock) { 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() - { - public void run() - { - lock.unlock(); - } - }); - } + this.lock = lock; } public Object getAttribute(String name) { - lock(); + lock.acquireWriteLock(); return session.getAttribute(name); } public List<String> getAttributeNames() { - lock(); + lock.acquireReadLock(); return InternalUtils.toList(session.getAttributeNames()); } public void setAttribute(String name, Object value) { - lock(); + lock.acquireWriteLock(); session.setAttribute(name, value); } public List<String> getAttributeNames(String prefix) { - lock(); + lock.acquireReadLock(); List<String> result = CollectionFactory.newList(); http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/680ffc3a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/SessionLock.java ---------------------------------------------------------------------- diff --git a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/SessionLock.java b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/SessionLock.java new file mode 100644 index 0000000..d234bd4 --- /dev/null +++ b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/SessionLock.java @@ -0,0 +1,36 @@ +// Copyright 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. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package org.apache.tapestry5.internal.services; + +/** + * A wrapper around {@link java.util.concurrent.locks.ReentrantReadWriteLock} used to manage the lock for a session. + * Once a lock is acquired, a callback is registered with the {@link org.apache.tapestry5.ioc.services.PerthreadManager} + * to release the lock at the end of the request. + * + * @since 5.4 + */ +public interface SessionLock +{ + /** + * Acquires the read lock, if the shared read lock, or exclusive write lock, is not already held by this thread. + */ + void acquireReadLock(); + + /** + * Acquires the exclusive write lock; may release the (shared) read lock before acquiring the write lock; + * this may block for a while. Does nothing if the write lock is already held by this thread. + */ + void acquireWriteLock(); +} http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/680ffc3a/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 6a38372..3db2d28 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 @@ -22,6 +22,11 @@ import org.apache.tapestry5.services.SessionPersistedObjectAnalyzer; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpSession; +import java.util.Map; +import java.util.WeakHashMap; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; public class TapestrySessionFactoryImpl implements TapestrySessionFactory { @@ -33,6 +38,74 @@ public class TapestrySessionFactoryImpl implements TapestrySessionFactory private final PerthreadManager perthreadManager; + private final Lock mapLock = new ReentrantLock(); + + private final Map<HttpSession, SessionLock> sessionToLock = new WeakHashMap<HttpSession, SessionLock>(); + + private class SessionLockImpl implements SessionLock + { + + private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock(); + + private boolean isReadLocked() + { + return lock.getReadHoldCount() != 0; + } + + private boolean isWriteLocked() + { + return lock.isWriteLockedByCurrentThread(); + } + + public void acquireReadLock() + { + if (isReadLocked() || isWriteLocked()) + { + return; + } + + lock.readLock().lock(); + + perthreadManager.addThreadCleanupCallback(new Runnable() + { + public void run() + { + // The read lock may have been released, if upgraded to a write lock. + if (isReadLocked()) + { + lock.readLock().unlock(); + } + } + }); + } + + public void acquireWriteLock() + { + if (isWriteLocked()) + { + return; + } + + if (isReadLocked()) + { + lock.readLock().unlock(); + } + + // During this window, no lock is held, and the next call may block. + + lock.writeLock().lock(); + + perthreadManager.addThreadCleanupCallback(new Runnable() + { + public void run() + { + // This is the only way a write lock is unlocked, so no check is needed. + lock.writeLock().unlock(); + } + }); + } + } + public TapestrySessionFactoryImpl( @Symbol(SymbolConstants.CLUSTERED_SESSIONS) boolean clustered, @@ -55,11 +128,36 @@ public class TapestrySessionFactoryImpl implements TapestrySessionFactory return null; } + SessionLock lock = lockForSession(httpSession); + if (clustered) { - return new ClusteredSessionImpl(request, httpSession, perthreadManager, analyzer); + return new ClusteredSessionImpl(request, httpSession, lock, analyzer); } - return new SessionImpl(request, httpSession, perthreadManager); + return new SessionImpl(request, httpSession, lock); + } + + private SessionLock lockForSession(HttpSession session) + { + // Because WeakHashMap does not look thread safe to me, we use an exclusive + // lock. + mapLock.lock(); + + try + { + SessionLock result = sessionToLock.get(session); + + if (result == null) + { + result = new SessionLockImpl(); + sessionToLock.put(session, result); + } + + return result; + } finally + { + mapLock.unlock(); + } } } http://git-wip-us.apache.org/repos/asf/tapestry-5/blob/680ffc3a/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 0014abc..4f1aebf 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 @@ -15,15 +15,8 @@ package org.apache.tapestry5.internal.services; 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; @@ -35,58 +28,9 @@ 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() + private SessionLock mockLock() { - 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(); + return newMock(SessionLock.class); } @Test @@ -94,19 +38,17 @@ public class SessionImplTest extends InternalBaseTestCase { Enumeration e = Collections.enumeration(Arrays.asList("fred", "barney")); HttpSession hs = mockHttpSession(); + SessionLock lock = mockLock(); - trainForLock(hs); - + lock.acquireReadLock(); expect(hs.getAttributeNames()).andReturn(e); replay(); - Session session = new SessionImpl(null, hs, perThreadManager); + Session session = new SessionImpl(null, hs, lock); assertEquals(session.getAttributeNames(), Arrays.asList("barney", "fred")); - assertTrue(lock.isLocked()); - verify(); } @@ -115,19 +57,18 @@ public class SessionImplTest extends InternalBaseTestCase { Enumeration e = Collections.enumeration(Arrays.asList("fred", "barney", "fanny")); HttpSession hs = mockHttpSession(); + SessionLock lock = mockLock(); - trainForLock(hs); + lock.acquireReadLock(); expect(hs.getAttributeNames()).andReturn(e); replay(); - Session session = new SessionImpl(null, hs, perThreadManager); + Session session = new SessionImpl(null, hs, lock); assertEquals(session.getAttributeNames("f"), Arrays.asList("fanny", "fred")); - assertTrue(lock.isLocked()); - verify(); } @@ -135,14 +76,13 @@ public class SessionImplTest extends InternalBaseTestCase public void invalidate() { HttpSession hs = mockHttpSession(); - - trainForLock(hs); + SessionLock lock = mockLock(); hs.invalidate(); replay(); - Session session = new SessionImpl(null, hs, perThreadManager); + Session session = new SessionImpl(null, hs, lock); session.invalidate(); @@ -154,13 +94,13 @@ public class SessionImplTest extends InternalBaseTestCase { HttpSession hs = mockHttpSession(); HttpServletRequest hsr = mockHttpServletRequest(); + SessionLock lock = mockLock(); train_getSession(hsr, false, hs); - trainForLock(hs); replay(); - Session session = new SessionImpl(hsr, hs, perThreadManager); + Session session = new SessionImpl(hsr, hs, lock); assertFalse(session.isInvalidated()); @@ -187,13 +127,12 @@ public class SessionImplTest extends InternalBaseTestCase { HttpSession hs = mockHttpSession(); int seconds = 999; - trainForLock(hs); hs.setMaxInactiveInterval(seconds); replay(); - Session session = new SessionImpl(null, hs, perThreadManager); + Session session = new SessionImpl(null, hs, null); session.setMaxInactiveInterval(seconds); @@ -205,15 +144,13 @@ public class SessionImplTest extends InternalBaseTestCase { HttpSession hs = mockHttpSession(); - trainForLock(hs); - int seconds = 999; expect(hs.getMaxInactiveInterval()).andReturn(seconds); replay(); - Session session = new SessionImpl(null, hs, perThreadManager); + Session session = new SessionImpl(null, hs, null); assertEquals(session.getMaxInactiveInterval(), seconds); @@ -227,13 +164,15 @@ public class SessionImplTest extends InternalBaseTestCase HttpServletRequest hsr = mockHttpServletRequest(); SessionPersistedObjectAnalyzer analyzer = newMock(SessionPersistedObjectAnalyzer.class); Object dirty = new Object(); + SessionLock lock = mockLock(); + + lock.acquireWriteLock(); - trainForLock(hs); train_getAttribute(hs, "dirty", dirty); replay(); - Session session = new ClusteredSessionImpl(hsr, hs, perThreadManager, analyzer); + Session session = new ClusteredSessionImpl(hsr, hs, lock, analyzer); assertSame(session.getAttribute("dirty"), dirty); @@ -243,6 +182,8 @@ public class SessionImplTest extends InternalBaseTestCase train_getSession(hsr, false, hs); + lock.acquireWriteLock(); + hs.setAttribute("dirty", dirty); replay();
