Use File.list and File.walk within a try with resource #5718
Conversation
The API contract of File.list and File.walk requires them to be closed after use.
Left out filter
Factored out deleteFile with better debug
| try | ||
| { | ||
| sweepFile(now, p); | ||
| if (!Files.deleteIfExists(p)) |
There was a problem hiding this comment.
Bzzzt. Do not pass go, do not collect $100. YOu can only delete it if it has expired!
| LOG.warn(e); | ||
| if (!Files.deleteIfExists(p)) | ||
| LOG.warn("Could not delete {}", p.getFileName()); | ||
| if (LOG.isDebugEnabled()) |
There was a problem hiding this comment.
So the debug output looking like the delete happened will be done even if the file wasn't actually deleted.
There was a problem hiding this comment.
factored out to a better common deleteFile method
| try | ||
| { | ||
| sweepFile(now, p); | ||
| if (!Files.deleteIfExists(p)) | ||
| LOG.warn("Could not delete {}", p.getFileName()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
OK I have switched back.
jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java
Show resolved
Hide resolved
Can delete files whilst walking
There was a problem hiding this comment.
I'm good with this.
The session specific behaviors that @janbartel pointed out seem important though.
| } | ||
| catch (Exception e) | ||
| { | ||
| LOG.warn(e); | ||
| } | ||
| } | ||
|
|
||
| public boolean isExpired(long now, Path p) |
There was a problem hiding this comment.
I don't see the benefit of splitting sweepFile() into isExpired() and then deleteFile() - it's more efficient just to use sweepFile().
There was a problem hiding this comment.
Ah now that we no longer collect before delete, there is none....
Restored sweepFile fixed minor code suggestions
The API contract of File.list and File.walk requires them to be closed after use.