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-47324] - Reduce usage of File.mkdirs() #3173

Merged
merged 12 commits into from Dec 14, 2017

Conversation

3 participants
@KrishanBhasin
Copy link
Contributor

KrishanBhasin commented Dec 4, 2017

See JENKINS-47324.

Proposed changelog entries

  • Reduce usage of renameTo()

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
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@dwnusbaum

@KrishanBhasin

This comment has been minimized.

Copy link
Contributor Author

KrishanBhasin commented Dec 4, 2017

I have opened this PR early despite the code clearly not being ready yet as I am looking for feedback as I go - this is quite a challenging change for me so any help is appreciated!

@dwnusbaum dwnusbaum self-requested a review Dec 4, 2017

@@ -1962,7 +1963,7 @@ public void renameTo(final FilePath target) throws IOException, InterruptedExcep
act(new SecureFileCallable<Void>() {
private static final long serialVersionUID = 1L;
public Void invoke(File f, VirtualChannel channel) throws IOException {
reading(f).renameTo(creating(new File(target.remote)));
Files.move(reading(f).toPath(), creating(new File(target.remote)).toPath(), StandardCopyOption.COPY_ATTRIBUTES, LinkOption.NOFOLLOW_LINKS);

This comment has been minimized.

@KrishanBhasin

KrishanBhasin Dec 4, 2017

Author Contributor

I don't try/catch here as Files.move() throws an IOException, which is what we would like to propagate out from this class.

This comment has been minimized.

@dwnusbaum

dwnusbaum Dec 5, 2017

Member

That's fine, but you do want to merge with master and use Util#fileToPath anywhere you use File#toPath so that the runtime exception InvalidPathException is wrapped as an IOException.

This comment has been minimized.

@dwnusbaum

dwnusbaum Dec 5, 2017

Member

Also, the documentation for Files#move doesn't include StandardCopyOption.COPY_ATTRIBUTES, so I'd remove it here. (I think most implementations of Files#move attempt to copy attributes by default.)

@@ -1121,7 +1122,7 @@ public URI toURI() throws IOException, InterruptedException {
return act(new SecureFileCallable<URI>() {
private static final long serialVersionUID = 1L;
public URI invoke(File f, VirtualChannel channel) {
return f.toURI();
return f.toPath().toUri();

This comment has been minimized.

@KrishanBhasin

KrishanBhasin Dec 4, 2017

Author Contributor

I don't see a tangible benefit of this change - can anyone explain? It is listed as the 'new' equivalent of the old method here

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Dec 4, 2017

Member

There is no strong benefit. However, in your case you are at risk of a new InvalidPathException, which may be thrown by toPath()

Replace mkdirs() with Files.createDirectories()
Replace mkdir() with Files.createTempDirectory()

@oleg-nenashev oleg-nenashev changed the title Jenkins Issue 47327 [JENKINS-47324] - Reduce usage of renameTo() Dec 4, 2017

@@ -1121,7 +1122,7 @@ public URI toURI() throws IOException, InterruptedException {
return act(new SecureFileCallable<URI>() {
private static final long serialVersionUID = 1L;
public URI invoke(File f, VirtualChannel channel) {
return f.toURI();
return f.toPath().toUri();

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Dec 4, 2017

Member

There is no strong benefit. However, in your case you are at risk of a new InvalidPathException, which may be thrown by toPath()

@@ -587,11 +588,11 @@ private void unzip(File dir, File zipFile) throws IOException {
ZipEntry e = entries.nextElement();
File f = new File(dir, e.getName());
if (e.isDirectory()) {
mkdirs(f);
Files.createDirectories(f.toPath());

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Dec 4, 2017

Member

Use Util#toPath() created by @dwnusbaum a while ago. Otherwise there is a risk of unhandled runtime InvalidPathException

This comment has been minimized.

@dwnusbaum

dwnusbaum Dec 5, 2017

Member

Also FilePath#mkdirs(File) calls filterNonNull().mkdirs(dir);, which checks permissions when doing remote operations. This removes that check. I would update the mkdirs method itself to use Files#createDirectories, and then you won't need to change all of its callers.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Dec 4, 2017

Marked the PR as "work-in-progress" according to the comment

@@ -1409,7 +1410,7 @@ public FilePath createTempDir(final String prefix, final String suffix) throws I
public String invoke(File dir, VirtualChannel channel) throws IOException {
File f = File.createTempFile(prefix, suffix, dir);
f.delete();
f.mkdir();
Files.createTempDirectory(f.toPath(), null);

This comment has been minimized.

@dwnusbaum

dwnusbaum Dec 5, 2017

Member

This function is being updated by #3161, so I'd remove this change as it will cause a merge conflict.

@@ -1962,7 +1963,7 @@ public void renameTo(final FilePath target) throws IOException, InterruptedExcep
act(new SecureFileCallable<Void>() {
private static final long serialVersionUID = 1L;
public Void invoke(File f, VirtualChannel channel) throws IOException {
reading(f).renameTo(creating(new File(target.remote)));
Files.move(reading(f).toPath(), creating(new File(target.remote)).toPath(), StandardCopyOption.COPY_ATTRIBUTES, LinkOption.NOFOLLOW_LINKS);

This comment has been minimized.

@dwnusbaum

dwnusbaum Dec 5, 2017

Member

That's fine, but you do want to merge with master and use Util#fileToPath anywhere you use File#toPath so that the runtime exception InvalidPathException is wrapped as an IOException.

@@ -587,11 +588,11 @@ private void unzip(File dir, File zipFile) throws IOException {
ZipEntry e = entries.nextElement();
File f = new File(dir, e.getName());
if (e.isDirectory()) {
mkdirs(f);
Files.createDirectories(f.toPath());

This comment has been minimized.

@dwnusbaum

dwnusbaum Dec 5, 2017

Member

Also FilePath#mkdirs(File) calls filterNonNull().mkdirs(dir);, which checks permissions when doing remote operations. This removes that check. I would update the mkdirs method itself to use Files#createDirectories, and then you won't need to change all of its callers.

@@ -1962,7 +1963,7 @@ public void renameTo(final FilePath target) throws IOException, InterruptedExcep
act(new SecureFileCallable<Void>() {
private static final long serialVersionUID = 1L;
public Void invoke(File f, VirtualChannel channel) throws IOException {
reading(f).renameTo(creating(new File(target.remote)));
Files.move(reading(f).toPath(), creating(new File(target.remote)).toPath(), StandardCopyOption.COPY_ATTRIBUTES, LinkOption.NOFOLLOW_LINKS);

This comment has been minimized.

@dwnusbaum

dwnusbaum Dec 5, 2017

Member

Also, the documentation for Files#move doesn't include StandardCopyOption.COPY_ATTRIBUTES, so I'd remove it here. (I think most implementations of Files#move attempt to copy attributes by default.)

@dwnusbaum dwnusbaum self-requested a review Dec 5, 2017

@KrishanBhasin

This comment has been minimized.

Copy link
Contributor Author

KrishanBhasin commented Dec 6, 2017

I had a crawl through how FilePath deletes files, and it looks like Util has methods that could be migrated to using the new Files.delete(Path) method?

@dwnusbaum

This comment has been minimized.

Copy link
Member

dwnusbaum commented Dec 6, 2017

@KrishanBhasin Yes definitely, I've been working on that in #3169 if you want to take a look and review it.

@dwnusbaum
Copy link
Member

dwnusbaum left a comment

The current state looks good. If you want to clean up the rest of the code that creates directories, you could also update IOUtils#mkdirs which is called by FilePath#mkdirsE, and then FilePath#mkdirs which has one last call to File#mkdirs (around line 1168) that could be switched to use mkdirs(f). That would get rid of all calls to File#mkdirs and File#mkdir (after #3161 is merged) from FilePath, which would be great.

import java.nio.file.Files;
import java.nio.file.InvalidPathException;
import java.nio.file.StandardCopyOption;
import java.nio.file.*;

This comment has been minimized.

@dwnusbaum

dwnusbaum Dec 7, 2017

Member

nit: I personally prefer using single-class imports (especially for non-static imports) to make it obvious what classes are being used and avoid any name clashes if new classes are added to the package. I don't know if there is a preference either way in Jenkins in general.

KrishanBhasin added some commits Dec 8, 2017

Remove * import.
move mkdirs() to using FilePath method instead of File method.
@KrishanBhasin

This comment has been minimized.

Copy link
Contributor Author

KrishanBhasin commented Dec 8, 2017

@dwnusbaum I think I've covered off those two methods you pointed out.

I thought this Jira ticket was going to involve a lot more changes, it looked a lot more intimidating than it actually was!

@KrishanBhasin KrishanBhasin changed the title [JENKINS-47324] - Reduce usage of renameTo() [JENKINS-47324] - Reduce usage of File.mkdirs() Dec 8, 2017

@oleg-nenashev
Copy link
Member

oleg-nenashev left a comment

Just another File#toPath()

} catch (InterruptedException e) {
// ignore
return Files.createDirectories(dir.toPath()).toFile();
} catch (UnsupportedOperationException e) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Dec 10, 2017

Member

Also https://docs.oracle.com/javase/7/docs/api/java/nio/file/InvalidPathException.html . @dwnusbaum has added a utility method for that recently, so you could replace dir.toPath() byt fileToPath(dir)

import static hudson.Util.deleteFile;
import static hudson.Util.fixEmpty;
import static hudson.Util.isSymlink;
import static hudson.Util.*;

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Dec 10, 2017

Member

I would recommend to avoid doing it in a production code to avoid ambiguity

This comment has been minimized.

@KrishanBhasin

KrishanBhasin Dec 10, 2017

Author Contributor

I thought I had set Intellij to avoid doing this! I have fixed the settings & the code

@oleg-nenashev
Copy link
Member

oleg-nenashev left a comment

👍

@@ -2957,11 +2960,12 @@ private File deleting(File f) {
return f;
}

private boolean mkdirs(File dir) {
private boolean mkdirs(File dir) throws IOException {
if (dir.exists()) return false;

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Dec 11, 2017

Member

I feel this method should check whether the file is a directory and throw the exception otherwise. It is not related to this change, I will follow-up in a pull request. Maybe we can kill the method at all to be on the safe side

// ignore
return Files.createDirectories(fileToPath(dir)).toFile();
} catch (UnsupportedOperationException e) {
throw new IOException(e);

This comment has been minimized.

@dwnusbaum

dwnusbaum Dec 12, 2017

Member

I'm not entirely sure what Ant <mkdir> task was referring to here before. It would be closer to the original code to try again if an IOException is thrown, but I can't think of a common error that would actually be fixed by doing that, so I'm ok with getting rid of the sleep with this change.

@oleg-nenashev oleg-nenashev merged commit 09bcc5d into jenkinsci:master Dec 14, 2017

1 check passed

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

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Dec 14, 2017

Thanks @KrishanBhasin !

@KrishanBhasin

This comment has been minimized.

Copy link
Contributor Author

KrishanBhasin commented Dec 15, 2017

Thanks @oleg-nenashev and @dwnusbaum for all the feedback as I went! I'll keep any eye out for another low hanging issue that I can try picking up :)

@KrishanBhasin KrishanBhasin deleted the KrishanBhasin:feature/47324-java-7-files-api branch Dec 16, 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.