From 7627b42b272b1388e25adc404393270d00e38430 Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 24 Nov 2020 13:29:31 +0100 Subject: [PATCH 1/5] Use File.list and File.walk within a try with resource The API contract of File.list and File.walk requires them to be closed after use. --- .../server/session/FileSessionDataStore.java | 161 ++++++++++-------- .../server/CustomResourcesMonitorTest.java | 3 +- .../org/eclipse/jetty/server/RequestTest.java | 5 +- .../util/RolloverFileOutputStreamTest.java | 7 +- .../jetty/util/resource/JarResourceTest.java | 6 +- .../jetty/webapp/WebAppContextTest.java | 12 +- .../jetty/server/session/FileTestHelper.java | 44 ++--- 7 files changed, 131 insertions(+), 107 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java index bcb8eef69cc7..0b2d5843224b 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java @@ -35,6 +35,8 @@ import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; +import java.util.stream.Stream; import org.eclipse.jetty.util.ClassLoadingObjectInputStream; import org.eclipse.jetty.util.MultiException; @@ -206,21 +208,24 @@ public void sweepDisk() long now = System.currentTimeMillis(); if (LOG.isDebugEnabled()) LOG.debug("Sweeping {} for old session files", _storeDir); - try + try (Stream stream = Files.walk(_storeDir.toPath(), 1, FileVisitOption.FOLLOW_LINKS)) { - Files.walk(_storeDir.toPath(), 1, FileVisitOption.FOLLOW_LINKS) + stream .filter(p -> !Files.isDirectory(p)).filter(p -> !isOurContextSessionFilename(p.getFileName().toString())) .filter(p -> isSessionFilename(p.getFileName().toString())) + .collect(Collectors.toList()) // Don't delete whilst walking .forEach(p -> { - try { - sweepFile(now, p); + if (!Files.deleteIfExists(p)) + LOG.warn("Could not delete {}", p.getFileName()); + if (LOG.isDebugEnabled()) + LOG.debug("Sweep deleted {}", p.getFileName()); } - catch (Exception e) + catch (IOException e) { - LOG.warn(e); + LOG.warn("Could not delete {}", p.getFileName(), e); } }); } @@ -230,6 +235,24 @@ public void sweepDisk() } } + public boolean isExpired(long now, Path p) + { + if (p == null) + return false; + try + { + long expiry = getExpiryFromFilename(p.getFileName().toString()); + //files with 0 expiry never expire + return expiry > 0 && ((now - expiry) >= (5 * TimeUnit.SECONDS.toMillis(_gracePeriodSec))); + } + catch (NumberFormatException e) + { + LOG.warn("Not valid session filename {}", p.getFileName()); + LOG.warn(e); + } + return false; + } + /** * Check to see if the expiry on the file is very old, and * delete the file if so. "Old" means that it expired at least @@ -242,24 +265,12 @@ public void sweepDisk() public void sweepFile(long now, Path p) throws Exception { - if (p == null) - return; - - try + if (isExpired(now, p)) { - long expiry = getExpiryFromFilename(p.getFileName().toString()); - //files with 0 expiry never expire - if (expiry > 0 && ((now - expiry) >= (5 * TimeUnit.SECONDS.toMillis(_gracePeriodSec)))) - { - Files.deleteIfExists(p); - if (LOG.isDebugEnabled()) - LOG.debug("Sweep deleted {}", p.getFileName()); - } - } - catch (NumberFormatException e) - { - LOG.warn("Not valid session filename {}", p.getFileName()); - LOG.warn(e); + if (!Files.deleteIfExists(p)) + LOG.warn("Could not delete {}", p.getFileName()); + if (LOG.isDebugEnabled()) + LOG.debug("Sweep deleted {}", p.getFileName()); } } @@ -365,70 +376,70 @@ public void initializeStore() MultiException me = new MultiException(); long now = System.currentTimeMillis(); - Files.walk(_storeDir.toPath(), 1, FileVisitOption.FOLLOW_LINKS) - .filter(p -> !Files.isDirectory(p)).filter(p -> isSessionFilename(p.getFileName().toString())) - .forEach(p -> - { - //first get rid of all ancient files, regardless of which - //context they are for - try - { - sweepFile(now, p); - } - catch (Exception x) - { - me.add(x); - } - String filename = p.getFileName().toString(); - String context = getContextFromFilename(filename); - //now process it if it wasn't deleted, and it is for our context - if (Files.exists(p) && _contextString.equals(context)) + // first get rid of all ancient files, regardless of which + // context they are for. Don't do in the following sweep to avoid + // deleting whilst walking. + sweepDisk(); + + // Build session file map by walking directory + try (Stream stream = Files.walk(_storeDir.toPath(), 1, FileVisitOption.FOLLOW_LINKS)) + { + stream + .filter(p -> !Files.isDirectory(p)) + .filter(p -> isSessionFilename(p.getFileName().toString())) + .forEach(p -> { - //the session is for our context, populate the map with it - String sessionIdWithContext = getIdWithContextFromFilename(filename); - if (sessionIdWithContext != null) + String filename = p.getFileName().toString(); + String context = getContextFromFilename(filename); + //now process it if it wasn't deleted, and it is for our context + if (Files.exists(p) && _contextString.equals(context)) { - //handle multiple session files existing for the same session: remove all - //but the file with the most recent expiry time - String existing = _sessionFileMap.putIfAbsent(sessionIdWithContext, filename); - if (existing != null) + //the session is for our context, populate the map with it + String sessionIdWithContext = getIdWithContextFromFilename(filename); + if (sessionIdWithContext != null) { - //if there was a prior filename, work out which has the most - //recent modify time - try + //handle multiple session files existing for the same session: remove all + //but the file with the most recent expiry time + String existing = _sessionFileMap.putIfAbsent(sessionIdWithContext, filename); + if (existing != null) { - long existingExpiry = getExpiryFromFilename(existing); - long thisExpiry = getExpiryFromFilename(filename); - - if (thisExpiry > existingExpiry) + //if there was a prior filename, work out which has the most + //recent modify time + try { - //replace with more recent file - Path existingPath = _storeDir.toPath().resolve(existing); - //update the file we're keeping - _sessionFileMap.put(sessionIdWithContext, filename); - //delete the old file - Files.delete(existingPath); - if (LOG.isDebugEnabled()) - LOG.debug("Replaced {} with {}", existing, filename); + long existingExpiry = getExpiryFromFilename(existing); + long thisExpiry = getExpiryFromFilename(filename); + + if (thisExpiry > existingExpiry) + { + //replace with more recent file + Path existingPath = _storeDir.toPath().resolve(existing); + //update the file we're keeping + _sessionFileMap.put(sessionIdWithContext, filename); + //delete the old file + Files.delete(existingPath); + if (LOG.isDebugEnabled()) + LOG.debug("Replaced {} with {}", existing, filename); + } + else + { + //we found an older file, delete it + Files.delete(p); + if (LOG.isDebugEnabled()) + LOG.debug("Deleted expired session file {}", filename); + } } - else + catch (IOException e) { - //we found an older file, delete it - Files.delete(p); - if (LOG.isDebugEnabled()) - LOG.debug("Deleted expired session file {}", filename); + me.add(e); } } - catch (IOException e) - { - me.add(e); - } } } - } - }); - me.ifExceptionThrow(); + }); + me.ifExceptionThrow(); + } } } diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/CustomResourcesMonitorTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/CustomResourcesMonitorTest.java index c3926e6761eb..7ff1f00add51 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/CustomResourcesMonitorTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/CustomResourcesMonitorTest.java @@ -146,9 +146,8 @@ public FileOnDirectoryMonitor(Path pathToMonitor) @Override public boolean isLowOnResources() { - try + try (Stream paths = Files.list(_pathToMonitor)) { - Stream paths = Files.list(_pathToMonitor); List content = paths.collect(Collectors.toList()); if (!content.isEmpty()) { diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java index 20486ca84b7a..f18c5aa6d809 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java @@ -36,6 +36,7 @@ import java.util.Locale; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Stream; import javax.servlet.DispatcherType; import javax.servlet.MultipartConfigElement; import javax.servlet.ServletException; @@ -1837,9 +1838,9 @@ public void testGetterSafeFromNullPointerException() private static long getFileCount(Path path) { - try + try (Stream s = Files.list(path)) { - return Files.list(path).count(); + return s.count(); } catch (IOException e) { diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/RolloverFileOutputStreamTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/RolloverFileOutputStreamTest.java index 75c494fa37a2..ab311d2a9475 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/RolloverFileOutputStreamTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/RolloverFileOutputStreamTest.java @@ -31,7 +31,7 @@ import java.util.Arrays; import java.util.TimeZone; import java.util.concurrent.TimeUnit; -import java.util.stream.Collectors; +import java.util.stream.Stream; import org.eclipse.jetty.toolchain.test.FS; import org.eclipse.jetty.toolchain.test.jupiter.WorkDir; @@ -379,6 +379,9 @@ private String readPath(Path path) throws IOException private String[] ls(Path path) throws IOException { - return Files.list(path).map(p -> p.getFileName().toString()).collect(Collectors.toList()).toArray(new String[0]); + try (Stream s = Files.list(path)) + { + return s.map(p -> p.getFileName().toString()).toArray(String[]::new); + } } } diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/resource/JarResourceTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/resource/JarResourceTest.java index db944868ea50..cc4a7ed917fa 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/resource/JarResourceTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/resource/JarResourceTest.java @@ -30,6 +30,7 @@ import java.util.List; import java.util.Set; import java.util.stream.Collectors; +import java.util.stream.Stream; import java.util.zip.ZipFile; import org.eclipse.jetty.toolchain.test.FS; @@ -264,7 +265,10 @@ public void testJarFileResourceListDirWithSpace() throws Exception private List listFiles(Path dir) throws IOException { - return Files.list(dir).collect(Collectors.toList()); + try (Stream s = Files.list(dir)) + { + return s.collect(Collectors.toList()); + } } private List listFiles(Path dir, DirectoryStream.Filter filter) throws IOException diff --git a/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/WebAppContextTest.java b/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/WebAppContextTest.java index 93f167564124..123955c2027c 100644 --- a/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/WebAppContextTest.java +++ b/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/WebAppContextTest.java @@ -365,10 +365,14 @@ public void testExtraClasspathGlob(String description, String extraClasspathGlob WebAppClassLoader webAppClassLoader = (WebAppClassLoader)contextClassLoader; Path extLibsDir = MavenTestingUtils.getTestResourcePathDir("ext"); extLibsDir = extLibsDir.toAbsolutePath(); - List expectedPaths = Files.list(extLibsDir) - .filter(Files::isRegularFile) - .filter((path) -> path.toString().endsWith(".jar")) - .collect(Collectors.toList()); + List expectedPaths; + try (Stream s = Files.list(extLibsDir)) + { + expectedPaths = s + .filter(Files::isRegularFile) + .filter((path) -> path.toString().endsWith(".jar")) + .collect(Collectors.toList()); + } List actualPaths = new ArrayList<>(); for (URL url : webAppClassLoader.getURLs()) { diff --git a/tests/test-sessions/test-file-sessions/src/test/java/org/eclipse/jetty/server/session/FileTestHelper.java b/tests/test-sessions/test-file-sessions/src/test/java/org/eclipse/jetty/server/session/FileTestHelper.java index a46322b59688..15f9d7538309 100644 --- a/tests/test-sessions/test-file-sessions/src/test/java/org/eclipse/jetty/server/session/FileTestHelper.java +++ b/tests/test-sessions/test-file-sessions/src/test/java/org/eclipse/jetty/server/session/FileTestHelper.java @@ -27,10 +27,10 @@ import java.io.OutputStream; import java.nio.file.Files; import java.nio.file.Path; -import java.util.List; import java.util.Map; import java.util.Optional; import java.util.stream.Collectors; +import java.util.stream.Stream; import org.eclipse.jetty.toolchain.test.FS; import org.eclipse.jetty.toolchain.test.jupiter.WorkDir; @@ -49,23 +49,28 @@ public class FileTestHelper public static File getFile(WorkDir workDir, String sessionId) throws IOException { - Optional sessionPath = Files.list(workDir.getPath()) - .filter((path) -> path.getFileName().toString().contains(sessionId)) - .findFirst(); - if (sessionPath.isPresent()) - return sessionPath.get().toFile(); - return null; + try (Stream s = Files.list(workDir.getPath())) + { + return s + .filter((path) -> path.getFileName().toString().contains(sessionId)) + .findFirst() + .map(Path::toFile) + .orElse(null); + } } public static void assertSessionExists(WorkDir workDir, String sessionId, boolean exists) throws IOException { - Optional sessionPath = Files.list(workDir.getPath()) - .filter((path) -> path.getFileName().toString().contains(sessionId)) - .findFirst(); - if (exists) - assertTrue(sessionPath.isPresent()); - else - assertFalse(sessionPath.isPresent()); + try (Stream s = Files.list(workDir.getPath())) + { + Optional sessionPath = s + .filter((path) -> path.getFileName().toString().contains(sessionId)) + .findFirst(); + if (exists) + assertTrue(sessionPath.isPresent()); + else + assertFalse(sessionPath.isPresent()); + } } public static void assertFileExists(WorkDir workDir, String filename, boolean exists) @@ -170,14 +175,11 @@ public static boolean checkSessionPersisted(WorkDir workDir, SessionData data) public static void deleteFile(WorkDir workDir, String sessionId) throws IOException { // Collect - List matches = Files.list(workDir.getPath()) - .filter((path) -> path.getFileName().toString().contains(sessionId)) - .collect(Collectors.toList()); - - // Delete outside of lambda - for (Path path : matches) + try (Stream s = Files.list(workDir.getPath())) { - FS.deleteFile(path); + s.filter((path) -> path.getFileName().toString().contains(sessionId)) + .collect(Collectors.toList()) // Delete outside of list stream + .forEach(FS::deleteFile); } } From dd196e7b8e5366c69bc83822dc183ff1915486a9 Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 24 Nov 2020 14:03:12 +0100 Subject: [PATCH 2/5] Fix from review Left out filter --- .../org/eclipse/jetty/server/session/FileSessionDataStore.java | 1 + 1 file changed, 1 insertion(+) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java index 0b2d5843224b..1aff47fcec1c 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java @@ -213,6 +213,7 @@ public void sweepDisk() stream .filter(p -> !Files.isDirectory(p)).filter(p -> !isOurContextSessionFilename(p.getFileName().toString())) .filter(p -> isSessionFilename(p.getFileName().toString())) + .filter(p -> isExpired(now, p)) .collect(Collectors.toList()) // Don't delete whilst walking .forEach(p -> { From 5a63e6005f46cbb6ad6e11cb981b1d4ec893128b Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 24 Nov 2020 14:09:17 +0100 Subject: [PATCH 3/5] Fix from review Factored out deleteFile with better debug --- .../server/session/FileSessionDataStore.java | 31 ++++++++----------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java index 1aff47fcec1c..1a1dc8747745 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java @@ -215,20 +215,7 @@ public void sweepDisk() .filter(p -> isSessionFilename(p.getFileName().toString())) .filter(p -> isExpired(now, p)) .collect(Collectors.toList()) // Don't delete whilst walking - .forEach(p -> - { - try - { - if (!Files.deleteIfExists(p)) - LOG.warn("Could not delete {}", p.getFileName()); - if (LOG.isDebugEnabled()) - LOG.debug("Sweep deleted {}", p.getFileName()); - } - catch (IOException e) - { - LOG.warn("Could not delete {}", p.getFileName(), e); - } - }); + .forEach(this::deleteFile); } catch (Exception e) { @@ -261,17 +248,25 @@ public boolean isExpired(long now, Path p) * * @param now the time now in msec * @param p the file to check - * @throws Exception indicating error in sweep */ public void sweepFile(long now, Path p) - throws Exception { if (isExpired(now, p)) + deleteFile(p); + } + + private void deleteFile(Path p) + { + try { if (!Files.deleteIfExists(p)) LOG.warn("Could not delete {}", p.getFileName()); - if (LOG.isDebugEnabled()) - LOG.debug("Sweep deleted {}", p.getFileName()); + else if (LOG.isDebugEnabled()) + LOG.debug("Deleted {}", p.getFileName()); + } + catch (IOException e) + { + LOG.warn("Could not delete {}", p.getFileName(), e); } } From a83462d46dfbb980fe3ca01542cba9e65b499227 Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 24 Nov 2020 14:13:37 +0100 Subject: [PATCH 4/5] Fix from review Can delete files whilst walking --- .../server/session/FileSessionDataStore.java | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java index 1a1dc8747745..5d0cf096dd1b 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java @@ -35,7 +35,6 @@ import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; -import java.util.stream.Collectors; import java.util.stream.Stream; import org.eclipse.jetty.util.ClassLoadingObjectInputStream; @@ -214,7 +213,6 @@ public void sweepDisk() .filter(p -> !Files.isDirectory(p)).filter(p -> !isOurContextSessionFilename(p.getFileName().toString())) .filter(p -> isSessionFilename(p.getFileName().toString())) .filter(p -> isExpired(now, p)) - .collect(Collectors.toList()) // Don't delete whilst walking .forEach(this::deleteFile); } catch (Exception e) @@ -372,12 +370,6 @@ public void initializeStore() MultiException me = new MultiException(); long now = System.currentTimeMillis(); - - // first get rid of all ancient files, regardless of which - // context they are for. Don't do in the following sweep to avoid - // deleting whilst walking. - sweepDisk(); - // Build session file map by walking directory try (Stream stream = Files.walk(_storeDir.toPath(), 1, FileVisitOption.FOLLOW_LINKS)) { @@ -386,6 +378,13 @@ public void initializeStore() .filter(p -> isSessionFilename(p.getFileName().toString())) .forEach(p -> { + // first get rid of all ancient files + if (isExpired(now, p)) + { + deleteFile(p); + return; + } + String filename = p.getFileName().toString(); String context = getContextFromFilename(filename); //now process it if it wasn't deleted, and it is for our context From 154c67913cd8ccf06fc423ae7c08d408a3a737b4 Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 24 Nov 2020 15:06:22 +0100 Subject: [PATCH 5/5] Fix from review Restored sweepFile fixed minor code suggestions --- .../server/session/FileSessionDataStore.java | 89 +++++++++---------- 1 file changed, 40 insertions(+), 49 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java index 5d0cf096dd1b..68284a51891b 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java @@ -154,7 +154,7 @@ public boolean deleteFile(String filename) throws Exception public Set doGetExpired(final Set candidates) { final long now = System.currentTimeMillis(); - HashSet expired = new HashSet(); + HashSet expired = new HashSet<>(); //iterate over the files and work out which have expired for (String filename : _sessionFileMap.values()) @@ -212,8 +212,7 @@ public void sweepDisk() stream .filter(p -> !Files.isDirectory(p)).filter(p -> !isOurContextSessionFilename(p.getFileName().toString())) .filter(p -> isSessionFilename(p.getFileName().toString())) - .filter(p -> isExpired(now, p)) - .forEach(this::deleteFile); + .forEach(p -> sweepFile(now, p)); } catch (Exception e) { @@ -221,24 +220,6 @@ public void sweepDisk() } } - public boolean isExpired(long now, Path p) - { - if (p == null) - return false; - try - { - long expiry = getExpiryFromFilename(p.getFileName().toString()); - //files with 0 expiry never expire - return expiry > 0 && ((now - expiry) >= (5 * TimeUnit.SECONDS.toMillis(_gracePeriodSec))); - } - catch (NumberFormatException e) - { - LOG.warn("Not valid session filename {}", p.getFileName()); - LOG.warn(e); - } - return false; - } - /** * Check to see if the expiry on the file is very old, and * delete the file if so. "Old" means that it expired at least @@ -249,22 +230,32 @@ public boolean isExpired(long now, Path p) */ public void sweepFile(long now, Path p) { - if (isExpired(now, p)) - deleteFile(p); - } - - private void deleteFile(Path p) - { - try + if (p != null) { - if (!Files.deleteIfExists(p)) - LOG.warn("Could not delete {}", p.getFileName()); - else if (LOG.isDebugEnabled()) - LOG.debug("Deleted {}", p.getFileName()); - } - catch (IOException e) - { - LOG.warn("Could not delete {}", p.getFileName(), e); + try + { + long expiry = getExpiryFromFilename(p.getFileName().toString()); + //files with 0 expiry never expire + if (expiry > 0 && ((now - expiry) >= (5 * TimeUnit.SECONDS.toMillis(_gracePeriodSec)))) + { + try + { + if (!Files.deleteIfExists(p)) + LOG.warn("Could not delete {}", p.getFileName()); + else if (LOG.isDebugEnabled()) + LOG.debug("Deleted {}", p.getFileName()); + } + catch (IOException e) + { + LOG.warn("Could not delete {}", p.getFileName(), e); + } + } + } + catch (NumberFormatException e) + { + LOG.warn("Not valid session filename {}", p.getFileName()); + LOG.warn(e); + } } } @@ -316,7 +307,7 @@ public SessionData doLoad(String id) throws Exception @Override public void doStore(String id, SessionData data, long lastSaveTime) throws Exception { - File file = null; + File file; if (_storeDir != null) { delete(id); @@ -333,8 +324,9 @@ public void doStore(String id, SessionData data, long lastSaveTime) throws Excep } catch (Exception e) { - if (file != null) - file.delete(); // No point keeping the file if we didn't save the whole session + // No point keeping the file if we didn't save the whole session + if (!file.delete()) + e.addSuppressed(new IOException("Could not delete " + file)); throw new UnwriteableSessionDataException(id, _context, e); } } @@ -358,7 +350,10 @@ public void initializeStore() throw new IllegalStateException("No file store specified"); if (!_storeDir.exists()) - _storeDir.mkdirs(); + { + if (!_storeDir.mkdirs()) + throw new IllegalStateException("Could not create " + _storeDir); + } else { if (!(_storeDir.isDirectory() && _storeDir.canWrite() && _storeDir.canRead())) @@ -379,11 +374,7 @@ public void initializeStore() .forEach(p -> { // first get rid of all ancient files - if (isExpired(now, p)) - { - deleteFile(p); - return; - } + sweepFile(now, p); String filename = p.getFileName().toString(); String context = getContextFromFilename(filename); @@ -517,11 +508,11 @@ protected String getIdFromFilename(String filename) protected long getExpiryFromFilename(String filename) { - if (StringUtil.isBlank(filename) || filename.indexOf("_") < 0) + if (StringUtil.isBlank(filename) || !filename.contains("_")) throw new IllegalStateException("Invalid or missing filename"); String s = filename.substring(0, filename.indexOf('_')); - return (s == null ? 0 : Long.parseLong(s)); + return Long.parseLong(s); } protected String getContextFromFilename(String filename) @@ -598,11 +589,11 @@ protected boolean isOurContextSessionFilename(String filename) protected SessionData load(InputStream is, String expectedId) throws Exception { - String id = null; //the actual id from inside the file + String id; //the actual id from inside the file try { - SessionData data = null; + SessionData data; DataInputStream di = new DataInputStream(is); id = di.readUTF();