-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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.
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
[JENKINS-48405] Use NIO in tryOnceDeleteFile and makeWritable #3169
Changes from all commits
bbe9471
1b029ef
3a4f6aa
4e59f20
cf41915
da94b18
94df2d8
816fc21
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,24 +28,19 @@ | |
|
||
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; | ||
import hudson.model.TaskListener; | ||
import hudson.os.PosixAPI; | ||
import hudson.util.QuotedStringTokenizer; | ||
import hudson.util.VariableResolver; | ||
|
||
import org.apache.commons.io.IOUtils; | ||
import org.apache.commons.lang.time.FastDateFormat; | ||
import org.apache.tools.ant.BuildException; | ||
import org.apache.tools.ant.Project; | ||
import org.apache.tools.ant.taskdefs.Chmod; | ||
import org.apache.tools.ant.taskdefs.Copy; | ||
import org.apache.tools.ant.types.FileSet; | ||
|
||
import org.kohsuke.accmod.Restricted; | ||
import org.kohsuke.accmod.restrictions.NoExternalUse; | ||
|
||
import jnr.posix.FileStat; | ||
import jnr.posix.POSIX; | ||
|
||
import javax.crypto.SecretKey; | ||
import javax.crypto.spec.SecretKeySpec; | ||
|
||
|
@@ -70,6 +65,7 @@ | |
import java.nio.file.Paths; | ||
import java.nio.file.attribute.PosixFilePermission; | ||
import java.nio.file.attribute.BasicFileAttributes; | ||
import java.nio.file.attribute.PosixFileAttributes; | ||
import java.nio.file.attribute.DosFileAttributes; | ||
import java.security.MessageDigest; | ||
import java.security.NoSuchAlgorithmException; | ||
|
@@ -257,16 +253,16 @@ public static void deleteFile(@Nonnull File f) throws IOException { | |
* | ||
* @param f | ||
* What to delete. If a directory, it'll need to be empty. | ||
* @throws IOException if it exists but could not be successfully deleted | ||
* @throws IOException if it exists but could not be successfully deleted, | ||
* or if it represents an invalid {@link Path}. | ||
*/ | ||
private static void tryOnceDeleteFile(File f) throws IOException { | ||
if (!f.delete()) { | ||
if(!f.exists()) | ||
// we are trying to delete a file that no longer exists, so this is not an error | ||
return; | ||
|
||
Path path = fileToPath(f); | ||
try { | ||
Files.deleteIfExists(path); | ||
} catch (IOException e) { | ||
// perhaps this file is read-only? | ||
makeWritable(f); | ||
makeWritable(path); | ||
/* | ||
on Unix both the file and the directory that contains it has to be writable | ||
for a file deletion to be successful. (Confirmed on Solaris 9) | ||
|
@@ -280,56 +276,47 @@ private static void tryOnceDeleteFile(File f) throws IOException { | |
$ rm x | ||
rm: x not removed: Permission denied | ||
*/ | ||
|
||
makeWritable(f.getParentFile()); | ||
|
||
if(!f.delete() && f.exists()) { | ||
// trouble-shooting. | ||
try { | ||
Files.deleteIfExists(f.toPath()); | ||
} catch (InvalidPathException e) { | ||
throw new IOException(e); | ||
} | ||
|
||
Path parent = path.getParent(); | ||
if (parent != null) { | ||
makeWritable(parent); | ||
} | ||
try { | ||
Files.deleteIfExists(path); | ||
} catch (IOException e2) { | ||
// see https://java.net/projects/hudson/lists/users/archive/2008-05/message/357 | ||
// I suspect other processes putting files in this directory | ||
File[] files = f.listFiles(); | ||
if(files!=null && files.length>0) | ||
throw new IOException("Unable to delete " + f.getPath()+" - files in dir: "+Arrays.asList(files)); | ||
throw new IOException("Unable to delete " + f.getPath()); | ||
throw new IOException("Unable to delete " + f.getPath()+" - files in dir: "+Arrays.asList(files), e2); | ||
throw e2; | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* Makes the given file writable by any means possible. | ||
* Makes the file at the given path writable by any means possible. | ||
*/ | ||
private static void makeWritable(@Nonnull File f) { | ||
if (f.setWritable(true)) { | ||
return; | ||
} | ||
// TODO do we still need to try anything else? | ||
|
||
// try chmod. this becomes no-op if this is not Unix. | ||
try { | ||
Chmod chmod = new Chmod(); | ||
chmod.setProject(new Project()); | ||
chmod.setFile(f); | ||
chmod.setPerm("u+w"); | ||
chmod.execute(); | ||
} catch (BuildException e) { | ||
LOGGER.log(Level.INFO,"Failed to chmod "+f,e); | ||
} | ||
|
||
try {// try libc chmod | ||
POSIX posix = PosixAPI.jnr(); | ||
String path = f.getAbsolutePath(); | ||
FileStat stat = posix.stat(path); | ||
posix.chmod(path, stat.mode()|0200); // u+w | ||
} catch (Throwable t) { | ||
LOGGER.log(Level.FINE,"Failed to chmod(2) "+f,t); | ||
private static void makeWritable(@Nonnull Path path) throws IOException { | ||
if (!Functions.isWindows()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we try to do something with |
||
try { | ||
PosixFileAttributes attrs = Files.readAttributes(path, PosixFileAttributes.class); | ||
Set<PosixFilePermission> newPermissions = ((PosixFileAttributes)attrs).permissions(); | ||
newPermissions.add(PosixFilePermission.OWNER_WRITE); | ||
Files.setPosixFilePermissions(path, newPermissions); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to |
||
return; | ||
} catch (NoSuchFileException e) { | ||
return; | ||
} catch (UnsupportedOperationException e) { | ||
// PosixFileAttributes not supported, fall back to old IO. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FINE or FINEST log? future diagnosis FTW. |
||
} | ||
} | ||
|
||
/** | ||
* We intentionally do not check the return code of setWritable, because if it | ||
* is false we prefer to rethrow the exception thrown by Files.deleteIfExists, | ||
* which will have a more useful message than something we make up here. | ||
*/ | ||
path.toFile().setWritable(true); | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,9 @@ | |
import java.io.FileInputStream; | ||
import java.io.IOException; | ||
import java.io.InputStream; | ||
import java.nio.file.FileSystemException; | ||
import java.nio.file.Files; | ||
import java.nio.file.Path; | ||
import java.nio.file.attribute.PosixFilePermissions; | ||
|
||
import static org.hamcrest.CoreMatchers.allOf; | ||
|
@@ -287,12 +290,6 @@ public void testDeleteFile() throws Exception { | |
@Test | ||
public void testDeleteFile_onWindows() throws Exception { | ||
Assume.assumeTrue(Functions.isWindows()); | ||
Class<?> c; | ||
try { | ||
c = Class.forName("java.nio.file.FileSystemException"); | ||
} catch (ClassNotFoundException x) { | ||
throw new AssumptionViolatedException("prior to JDK 7", x); | ||
} | ||
final int defaultDeletionMax = Util.DELETION_MAX; | ||
try { | ||
File f = tmp.newFile(); | ||
|
@@ -304,7 +301,7 @@ public void testDeleteFile_onWindows() throws Exception { | |
Util.deleteFile(f); | ||
fail("should not have been deletable"); | ||
} catch (IOException x) { | ||
assertThat(calcExceptionHierarchy(x), hasItem(c)); | ||
assertThat(calcExceptionHierarchy(x), hasItem(FileSystemException.class)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small cleanup since I was already looking at this test to address the failure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should ideally have been filed as a separate PR to assess not breaking pre-existing behaviour IMO. Or at least pushed as the first commit alone to trigger the PR build and see a green ✅ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reverted locally There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, I'll make sure to make test changes in a separate PR or push them as a first commit in the future. |
||
assertThat(x.getMessage(), containsString(f.getPath())); | ||
} | ||
} finally { | ||
|
@@ -313,6 +310,27 @@ public void testDeleteFile_onWindows() throws Exception { | |
} | ||
} | ||
|
||
@Test | ||
public void testDeleteFileReadOnly() throws Exception { | ||
// Removing the calls to Util#makeWritable in Util#tryOnceDeleteFile should cause this test to fail. | ||
Path file = tmp.newFolder().toPath().resolve("file.tmp"); | ||
Files.createDirectories(file.getParent()); | ||
Files.createFile(file); | ||
// Using old IO so the test can run on Windows. | ||
file.getParent().toFile().setWritable(false); | ||
file.toFile().setWritable(false); | ||
Util.deleteFile(file.toFile()); | ||
assertFalse(Files.exists(file)); | ||
} | ||
|
||
@Test | ||
public void testDeleteFileDoesNotExist() throws Exception { | ||
Path file = tmp.newFolder().toPath().resolve("file.tmp"); | ||
assertFalse(Files.exists(file)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NIT: |
||
// Should not throw an exception. | ||
Util.deleteFile(file.toFile()); | ||
} | ||
|
||
@Test | ||
public void testDeleteContentsRecursive() throws Exception { | ||
final File dir = tmp.newFolder(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly BTW, this method could log something by calling
java.nio.file.Files#isWritable
to help potential diagnosis?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that could be useful, or maybe
Util#tryOnceDeleteFile
could checkFiles#isWritable
instead of always calling this method and attempting the second delete. I am happy to handle it in a follow-up PR.