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.

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

Merged
merged 8 commits into from Dec 16, 2017

Conversation

5 participants
@dwnusbaum
Copy link
Member

commented Dec 1, 2017

See JENKINS-48405.

Proposed changelog entries

  • Use NIO to delete files for more robust error handling.

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
    • Needs testing to see how this impacts Unicode path issues such as JENKINS-12610: EDIT: It does not fix the issue.
    • Verify that edge cases (file doesn't exist/isn't writable) are handled by existing tests or create new tests to verify the behavior.
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@reviewbybees

LOGGER.log(Level.FINE,"Failed to chmod(2) "+f,t);
private static void makeWritable(@Nonnull Path path) throws IOException {
if (Functions.isWindows()) {
if (!path.toFile().setWritable(true)) {

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Dec 1, 2017

Author Member

Should we try using AclFileAttributeView before falling back to the old IO method?

}
} else {
try {
PosixFileAttributes attrs = Files.readAttributes(path, PosixFileAttributes.class);

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Dec 1, 2017

Author Member

Need to check whether we should be using LinkOption.NO_FOLLOW_LINKS.

try {
PosixFileAttributes attrs = Files.readAttributes(path, PosixFileAttributes.class);
Set<PosixFilePermission> newPermissions = ((PosixFileAttributes)attrs).permissions();
newPermissions.add(PosixFilePermission.OWNER_WRITE);

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Dec 1, 2017

Author Member

File.setWritable(true) sets OWNER_WRITE and OTHERS_WRITE, but the previous fallback methods only tried to set u+w. Should we add OTHERS_WRITE as well? I'll try to produce some test cases.

@dwnusbaum

This comment has been minimized.

Copy link
Member Author

commented Dec 1, 2017

Relevant test failure: hudson.UtilTest.testDeleteFile_onWindows

Expected: a collection containing <class java.nio.file.FileSystemException>
    but: was <class java.io.IOException>, was <class java.io.IOException>
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.junit.Assert.assertThat(Assert.java:956)
        at org.junit.Assert.assertThat(Assert.java:923)
        at hudson.UtilTest.testDeleteFile_onWindows(UtilTest.java:307)
@@ -304,7 +299,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));

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Dec 1, 2017

Author Member

Small cleanup since I was already looking at this test to address the failure.

This comment has been minimized.

Copy link
@batmat

batmat Dec 15, 2017

Member

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

This comment has been minimized.

Copy link
@batmat

batmat Dec 15, 2017

Member

Reverted locally src/main/java to master content, and the UtilTest still succeeds, so 👍.

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Dec 15, 2017

Author Member

Good point, I'll make sure to make test changes in a separate PR or push them as a first commit in the future.

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 new IOException("Unable to delete " + f.getPath(), e2);

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Dec 1, 2017

Author Member

There might be no point in the second wrapper here, since the top-level message thrown from Util#deleteFile already says Unable to delete <foo> Tried <n> times....

} 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()) {

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Dec 1, 2017

Author Member

Should we try to do something with AclFileAttributeView on Windows?

@dwnusbaum dwnusbaum changed the title [JENKINS-36088] Use NIO in tryOnceDeleteFile and makeWritable [JENKINS-48405] Use NIO in tryOnceDeleteFile and makeWritable Dec 6, 2017

@jglick jglick requested review from jglick and batmat Dec 6, 2017

@dwnusbaum dwnusbaum requested a review from jtnord Dec 6, 2017

@dwnusbaum dwnusbaum referenced this pull request Dec 6, 2017

Merged

[JENKINS-47324] - Reduce usage of File.mkdirs() #3173

0 of 4 tasks complete
@jglick

This comment has been minimized.

Copy link
Member

commented Dec 7, 2017

Did you mean to check

  • Needs testing to see how this impacts Unicode path issues such as JENKINS-12610

?

PosixFileAttributes attrs = Files.readAttributes(path, PosixFileAttributes.class);
Set<PosixFilePermission> newPermissions = ((PosixFileAttributes)attrs).permissions();
newPermissions.add(PosixFilePermission.OWNER_WRITE);
Files.setPosixFilePermissions(path, newPermissions);

This comment has been minimized.

Copy link
@jglick

jglick Dec 7, 2017

Member

Do we want to return here so as to skip the older File.setWritable call?

@dwnusbaum

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2017

Did you mean to check

Not yet, I am still testing that behavior.

@dwnusbaum

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2017

This does not fix the deletion issues regarding encoding, see my comment in JENKINS-12610.

@oleg-nenashev
Copy link
Member

left a comment

🐝 from what I see

@jglick jglick self-requested a review Dec 11, 2017

@jglick

jglick approved these changes Dec 11, 2017

@recampbell

This comment has been minimized.

Copy link
Member

commented Dec 12, 2017

@jglick

This comment has been minimized.

Copy link
Member

commented Dec 12, 2017

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Dec 14, 2017

My plan is to merge it tomorrow

@batmat

batmat approved these changes Dec 15, 2017

Copy link
Member

left a comment

🐝

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 {

This comment has been minimized.

Copy link
@batmat

batmat Dec 15, 2017

Member

Possibly BTW, this method could log something by calling java.nio.file.Files#isWritable to help potential diagnosis?

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Dec 15, 2017

Author Member

Yeah that could be useful, or maybe Util#tryOnceDeleteFile could check Files#isWritable instead of always calling this method and attempting the second delete. I am happy to handle it in a follow-up PR.

} catch (NoSuchFileException e) {
return;
} catch (UnsupportedOperationException e) {
// PosixFileAttributes not supported, fall back to old IO.

This comment has been minimized.

Copy link
@batmat

batmat Dec 15, 2017

Member

FINE or FINEST log? future diagnosis FTW.

@@ -304,7 +299,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));

This comment has been minimized.

Copy link
@batmat

batmat Dec 15, 2017

Member

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

@Test
public void testDeleteFileDoesNotExist() throws Exception {
Path file = tmp.newFolder().toPath().resolve("file.tmp");
assertFalse(Files.exists(file));

This comment has been minimized.

Copy link
@batmat

batmat Dec 15, 2017

Member

NIT: java.nio.file.Files#notExists also exists, FWIW.

@@ -304,7 +299,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));

This comment has been minimized.

Copy link
@batmat

batmat Dec 15, 2017

Member

Reverted locally src/main/java to master content, and the UtilTest still succeeds, so 👍.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Dec 16, 2017

Let's do changes in follow-ups

@oleg-nenashev oleg-nenashev merged commit 1270ba3 into jenkinsci:master Dec 16, 2017

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details

@dwnusbaum dwnusbaum deleted the dwnusbaum:JENKINS-36088 branch Dec 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.