BZ 69781 - improve FileStore thread safety

java/org/apache/catalina/session/FileStore.java

@@ -26,6 +26,9 @@
 import java.io.ObjectOutputStream;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 import jakarta.servlet.ServletContext;
 
@@ -71,6 +74,12 @@ public final class FileStore extends StoreBase {
     private File directoryFile = null;
 
 
+    /**
+     * A map of potential locks to control concurrent read/writes of a session's persistence file.
+     */
+    private final ConcurrentHashMap<String, UsageCountingReadWriteLock> idLocks = new ConcurrentHashMap();
+
+
     /**
      * Name to register for this Store, used for logging.
      */
@@ -183,7 +192,7 @@ public String[] keys() throws IOException {
     public Session load(String id) throws ClassNotFoundException, IOException {
         // Open an input stream to the specified pathname, if any
         File file = file(id);
-        if (file == null || !file.exists()) {
+        if (file == null) {
             return null;
         }
 
@@ -196,19 +205,26 @@ public Session load(String id) throws ClassNotFoundException, IOException {
 
         ClassLoader oldThreadContextCL = context.bind(Globals.IS_SECURITY_ENABLED, null);
 
-        try (FileInputStream fis = new FileInputStream(file.getAbsolutePath());
-                ObjectInputStream ois = getObjectInputStream(fis)) {
+        try {
+            acquireIdReadLock(id);
+            if (!file.exists()) {
+                return null;
+            }
 
-            StandardSession session = (StandardSession) manager.createEmptySession();
-            session.readObjectData(ois);
-            session.setManager(manager);
-            return session;
-        } catch (FileNotFoundException e) {
-            if (contextLog.isDebugEnabled()) {
-                contextLog.debug(sm.getString("fileStore.noFile", id, file.getAbsolutePath()));
+            try (FileInputStream fis = new FileInputStream(file.getAbsolutePath());
+                    ObjectInputStream ois = getObjectInputStream(fis)) {
+                StandardSession session = (StandardSession) manager.createEmptySession();
+                session.readObjectData(ois);
+                session.setManager(manager);
+                return session;
+            } catch (FileNotFoundException e) {
+                if (contextLog.isDebugEnabled()) {
+                    contextLog.debug(sm.getString("fileStore.noFile", id, file.getAbsolutePath()));
+                }
+                return null;
             }
-            return null;
         } finally {
+            releaseIdReadLock(id);
             context.unbind(Globals.IS_SECURITY_ENABLED, oldThreadContextCL);
         }
     }
@@ -225,8 +241,13 @@ public void remove(String id) throws IOException {
                     .trace(sm.getString(getStoreName() + ".removing", id, file.getAbsolutePath()));
         }
 
-        if (file.exists() && !file.delete()) {
-            throw new IOException(sm.getString("fileStore.deleteSessionFailed", file));
+        try{
+            acquireIdWriteLock(id);
+            if (file.exists() && !file.delete()) {
+                throw new IOException(sm.getString("fileStore.deleteSessionFailed", file));
+            }
+        } finally {
+            releaseIdWriteLock(id);
         }
     }
 
@@ -243,9 +264,14 @@ public void save(Session session) throws IOException {
                     .trace(sm.getString(getStoreName() + ".saving", session.getIdInternal(), file.getAbsolutePath()));
         }
 
-        try (FileOutputStream fos = new FileOutputStream(file.getAbsolutePath());
-                ObjectOutputStream oos = new ObjectOutputStream(new BufferedOutputStream(fos))) {
-            ((StandardSession) session).writeObjectData(oos);
+        try {
+            acquireIdWriteLock(session.getIdInternal());
+            try (FileOutputStream fos = new FileOutputStream(file.getAbsolutePath());
+                    ObjectOutputStream oos = new ObjectOutputStream(new BufferedOutputStream(fos))) {
+                ((StandardSession) session).writeObjectData(oos);
+            }
+        } finally {
+            releaseIdWriteLock(session.getIdInternal());
         }
     }
 
@@ -307,4 +333,85 @@ private File file(String id) throws IOException {
 
         return canonicalFile;
     }
+
+
+    /**
+     * Acquire and create if necessary a readlock for a given session id.
+     *
+     * @param id The ID of the Session.
+     */
+    private void acquireIdReadLock(String id) {
+        idLocks.compute(id,
+                (k, v) -> v == null ? new UsageCountingReadWriteLock() : v).lockRead();
+    }
+
+
+    /**
+     * Release a readlock for a given session id.
+     *
+     * @param id The ID of the Session.
+     */
+    private void releaseIdReadLock(String id) {
+        idLocks.computeIfPresent(id,
+                (k, v) -> v.releaseRead() == 0 ? null : v);
+    }
+
+
+    /**
+     * Acquire and create if necessary a writelock for a given session id.
+     *
+     * @param id The ID of the Session.
+     */
+    private void acquireIdWriteLock(String id) {
+        idLocks.compute(id,
+                (k, v) -> v == null ? new UsageCountingReadWriteLock() : v).lockWrite();
+    }
+
+
+    /**
+     * Release a writelock for a given session id.
+     *
+     * @param id The ID of the Session.
+     */
+    private void releaseIdWriteLock(String id) {
+        idLocks.computeIfPresent(id,
+                (k, v) -> v.releaseWrite() == 0 ? null : v);
+    }
+
+
+    /*
+     * The FileStore uses a per session ReentrantReadWriteLock to ensure that only one write (from a remove or save)
+     * occurs to a session persistence file at a time and not during any reads of the file (from a load call).  This
+     * is to protect from concurrency issues that may arise, particularly if using a PersistentValve. To limit the
+     * size of the session ID to lock map, the locks are created when required and destroyed (made eligible for GC)
+     * as soon as they are not required.
+     */
+    private static class UsageCountingReadWriteLock {
+        private final AtomicLong usageCount = new AtomicLong(0);
+        private final ReentrantReadWriteLock lock;
+
+        private UsageCountingReadWriteLock() {
+            lock = new ReentrantReadWriteLock();
+        }
+
+        private void lockRead() {
+            usageCount.incrementAndGet();
+            lock.readLock().lock();
+        }
+
+        private long releaseRead() {
+            lock.readLock().unlock();
+            return usageCount.decrementAndGet();
+        }
+
+        private void lockWrite() {
+            usageCount.incrementAndGet();
+            lock.writeLock().lock();
+        }
+
+        private long releaseWrite() {
+            lock.writeLock().unlock();
+            return usageCount.decrementAndGet();
+        }
+    }
 }