Skip to content

Commit

Permalink
Use File.list and File.walk within a try with resource (#5718)
Browse files Browse the repository at this point in the history 
* 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.

* Fix from review

Left out filter

* Fix from review

Factored out deleteFile with better debug

* Fix from review

Can delete files whilst walking

* Fix from review

Restored sweepFile
fixed minor code suggestions
  • Loading branch information
@gregw
gregw committed Nov 24, 2020
1 parent d0a7c88 commit 9f82ca0
Show file tree
Hide file tree
Showing 7 changed files with 133 additions and 123 deletions.
179 changes: 88 additions & 91 deletions jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java
View file
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 @@ -153,7 +154,7 @@ public boolean deleteFile(String filename) throws Exception
public Set<String> doGetExpired(final Set<String> candidates)
{
final long now = System.currentTimeMillis();
HashSet<String> expired = new HashSet<String>();
HashSet<String> expired = new HashSet<>();

//iterate over the files and work out which have expired
for (String filename : _sessionFileMap.values())
Expand Down Expand Up @@ -206,23 +207,12 @@ 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);
}
});
.forEach(p -> sweepFile(now, p));
}
catch (Exception e)
{
Expand All @@ -237,30 +227,36 @@ public void sweepDisk()
*
* @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;

try
if (p != null)
{
long expiry = getExpiryFromFilename(p.getFileName().toString());
//files with 0 expiry never expire
if (expiry > 0 && ((now - expiry) >= (5 * TimeUnit.SECONDS.toMillis(_gracePeriodSec))))
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)
{
Files.deleteIfExists(p);
if (LOG.isDebugEnabled())
LOG.debug("Sweep deleted {}", p.getFileName());
LOG.warn("Not valid session filename {}", p.getFileName());
LOG.warn(e);
}
}
catch (NumberFormatException e)
{
LOG.warn("Not valid session filename {}", p.getFileName());
LOG.warn(e);
}
}

@Override
Expand Down Expand Up @@ -311,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);
Expand All @@ -328,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);
}
}
Expand All @@ -353,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()))
Expand All @@ -365,70 +365,67 @@ 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
// 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 ->
{
// first get rid of all ancient files
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))
{
//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();
}
}
}

Expand Down Expand Up @@ -511,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)
Expand Down Expand Up @@ -592,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();
Expand Down
3 changes: 1 addition & 2 deletions jetty-server/src/test/java/org/eclipse/jetty/server/CustomResourcesMonitorTest.java
View file
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
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
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
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
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

0 comments on commit 9f82ca0

Please sign in to comment.