This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/10.1.x by this push: new 3e8d12f176 Fix BZ 69781 - make load, save and remove thread-safe 3e8d12f176 is described below commit 3e8d12f1761b267270c3f031d8828837899da026 Author: Mark Thomas <ma...@apache.org> AuthorDate: Wed Aug 27 10:55:23 2025 +0100 Fix BZ 69781 - make load, save and remove thread-safe https://bz.apache.org/bugzilla/show_bug.cgi?id=69781 --- java/org/apache/catalina/session/FileStore.java | 33 ++++++++++++++++++---- ...currency.java => TestFileStoreConcurrency.java} | 2 +- webapps/docs/changelog.xml | 6 ++++ 3 files changed, 34 insertions(+), 7 deletions(-) diff --git a/java/org/apache/catalina/session/FileStore.java b/java/org/apache/catalina/session/FileStore.java index 375de3df4d..bc183e09d6 100644 --- a/java/org/apache/catalina/session/FileStore.java +++ b/java/org/apache/catalina/session/FileStore.java @@ -26,6 +26,7 @@ import java.io.ObjectInputStream; import java.io.ObjectOutputStream; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.locks.Lock; import jakarta.servlet.ServletContext; @@ -34,6 +35,7 @@ import org.apache.catalina.Globals; import org.apache.catalina.Session; import org.apache.juli.logging.Log; import org.apache.juli.logging.LogFactory; +import org.apache.tomcat.util.concurrent.KeyedReentrantReadWriteLock; import org.apache.tomcat.util.res.StringManager; /** @@ -70,6 +72,7 @@ public final class FileStore extends StoreBase { */ private File directoryFile = null; + private KeyedReentrantReadWriteLock sessionLocksById = new KeyedReentrantReadWriteLock(); /** * Name to register for this Store, used for logging. @@ -183,7 +186,7 @@ public final class FileStore extends StoreBase { 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,8 +199,13 @@ public final class FileStore extends StoreBase { ClassLoader oldThreadContextCL = context.bind(Globals.IS_SECURITY_ENABLED, null); + Lock readLock = sessionLocksById.getLock(id).readLock(); + readLock.lock(); try (FileInputStream fis = new FileInputStream(file.getAbsolutePath()); ObjectInputStream ois = getObjectInputStream(fis)) { + if (!file.exists()) { + return null; + } StandardSession session = (StandardSession) manager.createEmptySession(); session.readObjectData(ois); @@ -209,6 +217,7 @@ public final class FileStore extends StoreBase { } return null; } finally { + readLock.unlock(); context.unbind(Globals.IS_SECURITY_ENABLED, oldThreadContextCL); } } @@ -225,8 +234,14 @@ public final class FileStore extends StoreBase { .trace(sm.getString(getStoreName() + ".removing", id, file.getAbsolutePath())); } - if (file.exists() && !file.delete()) { - throw new IOException(sm.getString("fileStore.deleteSessionFailed", file)); + Lock writeLock = sessionLocksById.getLock(id).writeLock(); + writeLock.lock(); + try { + if (file.exists() && !file.delete()) { + throw new IOException(sm.getString("fileStore.deleteSessionFailed", file)); + } + } finally { + writeLock.unlock(); } } @@ -243,9 +258,15 @@ public final class FileStore extends StoreBase { .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); + Lock writeLock = sessionLocksById.getLock(session.getIdInternal()).writeLock(); + writeLock.lock(); + try { + try (FileOutputStream fos = new FileOutputStream(file.getAbsolutePath()); + ObjectOutputStream oos = new ObjectOutputStream(new BufferedOutputStream(fos))) { + ((StandardSession) session).writeObjectData(oos); + } + } finally { + writeLock.unlock(); } } diff --git a/test/org/apache/catalina/session/TesterFileStoreConcurrency.java b/test/org/apache/catalina/session/TestFileStoreConcurrency.java similarity index 99% rename from test/org/apache/catalina/session/TesterFileStoreConcurrency.java rename to test/org/apache/catalina/session/TestFileStoreConcurrency.java index 809c213061..502605f731 100644 --- a/test/org/apache/catalina/session/TesterFileStoreConcurrency.java +++ b/test/org/apache/catalina/session/TestFileStoreConcurrency.java @@ -37,7 +37,7 @@ import org.apache.tomcat.unittest.TesterContext; * This needs to be run manually. It will not run as part of the standard unit tests as it is named "Tester...". This * could be changed once the bug has been fixed. */ -public class TesterFileStoreConcurrency { +public class TestFileStoreConcurrency { private static final int TEST_RUN_TIME_MS = 5000; diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 5b509184fb..7eb05ab6b1 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -111,6 +111,12 @@ Remove a number of unnecessary packages from the catalina-deployer.jar. (markt) </scode> + <fix> + <bug>69781</bug>: Fix concurrent access issues in the session + <code>FileStore</code> implementation that were causing lost sessions + when the store was used with the <code>PersistentValve</code>. Based on + pull request <pr>882</pr> by Aaron Ogburn. (markt) + </fix> </changelog> </subsection> <subsection name = "Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org