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 4 commits
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
168 changes: 87 additions & 81 deletions jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java
View file
Open in desktop
Expand Up @@ -35,6 +35,7 @@
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;
import java.util.stream.Stream;

import org.eclipse.jetty.util.ClassLoadingObjectInputStream;
import org.eclipse.jetty.util.MultiException;
Expand Down Expand Up @@ -206,60 +207,64 @@ 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()))
.forEach(p ->
{

try
{
sweepFile(now, p);
}
catch (Exception e)
{
LOG.warn(e);
}
});
.filter(p -> isExpired(now, p))
.forEach(this::deleteFile);
}
catch (Exception e)
{
LOG.warn(e);
}
}

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
* 5 gracePeriods ago. The session can belong to any context.
*
* @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 (p == null)
return;
if (isExpired(now, p))
deleteFile(p);
}

private void deleteFile(Path p)
{
try
{
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());
}
if (!Files.deleteIfExists(p))
LOG.warn("Could not delete {}", p.getFileName());
else if (LOG.isDebugEnabled())
LOG.debug("Deleted {}", p.getFileName());
}
catch (NumberFormatException e)
catch (IOException e)
{
LOG.warn("Not valid session filename {}", p.getFileName());
LOG.warn(e);
LOG.warn("Could not delete {}", p.getFileName(), e);
}
}

Expand Down Expand Up @@ -365,70 +370,71 @@ 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)
// 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 ->
{
me.add(x);
}
// 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
if (Files.exists(p) && _contextString.equals(context))
{
//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