Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use File.list and File.walk within a try with resource #5718

Merged
merged 5 commits into from Nov 24, 2020
Merged

Use File.list and File.walk within a try with resource #5718

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7627b42
Use File.list and File.walk within a try with resource
gregw Nov 24, 2020
dd196e7
Fix from review
gregw Nov 24, 2020
5a63e60
Fix from review
gregw Nov 24, 2020
a83462d
Fix from review
gregw Nov 24, 2020
154c679
Fix from review
gregw Nov 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
161 changes: 86 additions & 75 deletions jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -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<Path> 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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bzzzt. Do not pass go, do not collect $100. YOu can only delete it if it has expired!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oooops fixed

LOG.warn("Could not delete {}", p.getFileName());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I'm not sure we need to do the delete outside of the stream: there are plenty of examples if you do a google search where the recommended way to do a recursive directory delete is to use Files.walk/list and delete each file. I can't find any counter examples that says that is a bad idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I have switched back.

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);
}
});
}
Expand All @@ -230,6 +235,24 @@ public void sweepDisk()
}
}

public boolean isExpired(long now, Path p)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the benefit of splitting sweepFile() into isExpired() and then deleteFile() - it's more efficient just to use sweepFile().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah now that we no longer collect before delete, there is none....

{
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
Expand All @@ -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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the debug output looking like the delete happened will be done even if the file wasn't actually deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

factored out to a better common deleteFile method

LOG.debug("Sweep deleted {}", p.getFileName());
}
}

Expand Down Expand Up @@ -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<Path> 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);
janbartel marked this conversation as resolved.
Show resolved Hide resolved
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();
}
}
}

Expand Down
3 changes: 1 addition & 2 deletions jetty-server/src/test/java/org/eclipse/jetty/server/CustomResourcesMonitorTest.java
View file
Open in desktop
Expand Up @@ -146,9 +146,8 @@ public FileOnDirectoryMonitor(Path pathToMonitor)
@Override
public boolean isLowOnResources()
{
try
try (Stream<Path> paths = Files.list(_pathToMonitor))
{
Stream<Path> paths = Files.list(_pathToMonitor);
List<Path> content = paths.collect(Collectors.toList());
if (!content.isEmpty())
{
Expand Down
5 changes: 3 additions & 2 deletions jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -1837,9 +1838,9 @@ public void testGetterSafeFromNullPointerException()

private static long getFileCount(Path path)
{
try
try (Stream<Path> s = Files.list(path))
{
return Files.list(path).count();
return s.count();
}
catch (IOException e)
{
Expand Down
7 changes: 5 additions & 2 deletions jetty-util/src/test/java/org/eclipse/jetty/util/RolloverFileOutputStreamTest.java
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -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<Path> s = Files.list(path))
{
return s.map(p -> p.getFileName().toString()).toArray(String[]::new);
}
}
}
6 changes: 5 additions & 1 deletion jetty-util/src/test/java/org/eclipse/jetty/util/resource/JarResourceTest.java
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -264,7 +265,10 @@ public void testJarFileResourceListDirWithSpace() throws Exception

private List<Path> listFiles(Path dir) throws IOException
{
return Files.list(dir).collect(Collectors.toList());
try (Stream<Path> s = Files.list(dir))
{
return s.collect(Collectors.toList());
}
}

private List<Path> listFiles(Path dir, DirectoryStream.Filter<? super Path> filter) throws IOException
Expand Down
12 changes: 8 additions & 4 deletions jetty-webapp/src/test/java/org/eclipse/jetty/webapp/WebAppContextTest.java
View file
Open in desktop
Expand Up @@ -365,10 +365,14 @@ public void testExtraClasspathGlob(String description, String extraClasspathGlob
WebAppClassLoader webAppClassLoader = (WebAppClassLoader)contextClassLoader;
Path extLibsDir = MavenTestingUtils.getTestResourcePathDir("ext");
extLibsDir = extLibsDir.toAbsolutePath();
List<Path> expectedPaths = Files.list(extLibsDir)
.filter(Files::isRegularFile)
.filter((path) -> path.toString().endsWith(".jar"))
.collect(Collectors.toList());
List<Path> expectedPaths;
try (Stream<Path> s = Files.list(extLibsDir))
{
expectedPaths = s
.filter(Files::isRegularFile)
.filter((path) -> path.toString().endsWith(".jar"))
.collect(Collectors.toList());
}
List<Path> actualPaths = new ArrayList<>();
for (URL url : webAppClassLoader.getURLs())
{
Expand Down
44 changes: 23 additions & 21 deletions ...ons/test-file-sessions/src/test/java/org/eclipse/jetty/server/session/FileTestHelper.java
View file
Open in desktop
Expand Up @@ -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;
Expand All @@ -49,23 +49,28 @@ public class FileTestHelper

public static File getFile(WorkDir workDir, String sessionId) throws IOException
{
Optional<Path> sessionPath = Files.list(workDir.getPath())
.filter((path) -> path.getFileName().toString().contains(sessionId))
.findFirst();
if (sessionPath.isPresent())
return sessionPath.get().toFile();
return null;
try (Stream<Path> 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<Path> sessionPath = Files.list(workDir.getPath())
.filter((path) -> path.getFileName().toString().contains(sessionId))
.findFirst();
if (exists)
assertTrue(sessionPath.isPresent());
else
assertFalse(sessionPath.isPresent());
try (Stream<Path> s = Files.list(workDir.getPath()))
{
Optional<Path> 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)
Expand Down Expand Up @@ -170,14 +175,11 @@ public static boolean checkSessionPersisted(WorkDir workDir, SessionData data)
public static void deleteFile(WorkDir workDir, String sessionId) throws IOException
{
// Collect
List<Path> 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<Path> 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);
}
}

Expand Down