[JENKINS-42934] Avoid using new FileInputStream / new FileOutputStream #2816

stephenc · 2017-03-20T12:51:49Z


        [JENKINS-42934] Avoid using new FileInputStream / new FileOutputStream

stephenc · 2017-03-20T12:52:42Z

Also @jenkinsci/code-reviewers

reviewbybees · 2017-03-20T12:58:56Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

MarkEWaite · 2017-03-20T14:16:53Z

It looks like the same change is needed in git client plugin for its use of new FileInputStream. Nice catch

jglick

Sounds good to me but should there be a FindBugs rule for this? Or should we let the JDK team decide whether these classes should be deprecated unless and until a proper fix is made? (In practice they seem very reluctant to deprecate anything, even in cases where there is a demonstrable danger.)

stephenc · 2017-03-20T15:51:15Z

Some of the test failures seem legitimate


        [JENKINS-42934] Need to catch a different exception

stephenc · 2017-03-20T15:54:33Z

@jglick a findbugs rule to catch any use of FileInputStream / FileOutputStream would probably be a good idea for a follow-up... as would corresponding changes in all plugins... but let's get this change tested and even better if we can run it through the ATH (not sure how to do that as the ATH has never worked on my machine)


        [JENKINS-42934] When you specify options you need to include CREATE o…

…r the file will not be created


        [JENKINS-42934] Actually use Java's file locking API to lock the file…

… on windows

oleg-nenashev

The most of the changes look good to me, but the approach in UtilTest#lockFileForDeletion() may likely cause instability in tests according to my experience with Java FileChannel API. Hence 🐛 though it is a weak one (test code)

oleg-nenashev · 2017-03-21T11:47:24Z

core/src/test/java/hudson/UtilTest.java

@@ -490,9 +494,13 @@ private void lockFileForDeletion(File f) throws IOException, InterruptedExceptio
        // On unix, can't use "chattr +u" because ext fs ignores it.
        // On Windows, we can't delete files that are open for reading, so we use that.
        assert Functions.isWindows();
-        final InputStream s = new FileInputStream(f);
+        final FileChannel channel = FileChannel.open(f.toPath(), StandardOpenOption.READ, StandardOpenOption.WRITE);
+        final FileLock lock = channel.lock();


🐛 Such approach may cause the OverlappingFileLockException if the lock is being already held by the JVM. It will cause an unhadled and random Runtime exception here. The best pattern is a loop with tryLock(), but surprisingly it also has to handle this exception.

How is this worse than the code that was there before?

The point of code review is not to get the reviewed code perfect, but to ensure that the code quality is an increasing function. i.e. dQ/dC < 0 is grounds to put a bug but dQ/dC == 0 is not (though dQ/dC > 0 is preferred obviously)

If we keep the attitude that dQ/dC == ∞ then we will never make any changes

(where Q is quality and C is changes to the code)

With this code, if run on a windows filesystem that does not support locking we will get a test Error rather than a test Failure... which IMHO is actually the correct result... so to put the point further, I would argue that this is actually the correct behaviour from test code (although my intent had been like for like so this is more of an unintended virtuous side-effect ;-) )

Likely it was a response to #2816 (comment), or maybe I do not get the response

OverlappingFileLockException will be happening on the system, which does support file locking. And, ridiculously, these exceptions happen on the success path (JVMs synchronize between each other, but JVM does not sync locks within itself. likely a bad design, but as designed).

oleg-nenashev · 2017-03-21T11:47:24Z

core/src/test/java/hudson/UtilTest.java

@@ -490,9 +494,13 @@ private void lockFileForDeletion(File f) throws IOException, InterruptedExceptio
        // On unix, can't use "chattr +u" because ext fs ignores it.
        // On Windows, we can't delete files that are open for reading, so we use that.
        assert Functions.isWindows();
-        final InputStream s = new FileInputStream(f);
+        final FileChannel channel = FileChannel.open(f.toPath(), StandardOpenOption.READ, StandardOpenOption.WRITE);


Though it is correct change for file locking API, it follows the presumption that All windows FileSystems support locking. And it is not correct. If the locking is not supported, IIRC the lock() call throws the IOException, which goes outside the method. Maybe it would be better to check if locking is supported in assert/assume

oleg-nenashev · 2017-03-21T11:47:24Z

core/src/main/java/hudson/util/io/RewindableFileOutputStream.java

-                    current = new FileOutputStream(out,false);
-                } catch (FileNotFoundException e) {
+                    current = Files.newOutputStream(out.toPath(), StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING);
+                } catch (FileNotFoundException | NoSuchFileException e) {


Hmm, why? I would understand the InvalidPathException: https://docs.oracle.com/javase/7/docs/api/java/io/File.html#toPath()

Other calls do not also catch it

It's from Files.newOutputStream IIUC

Checked exception thrown when an attempt is made to access a file that does not exist.

So basically it seems that the JVM had NIH with itself

Uh oh, you are right. http://stackoverflow.com/questions/14263725/java-files-write-nosuchfileexception. Does it make sense to catch it in other usages then?


        [JENKINS-42934] A couple of places where FileNotFoundException may be…

… replaced by NoSuchFileException by JVM shenanigans


        [JENKINS-42934] Some unit tests need side-effects of FileInputStream

oleg-nenashev

🐝 since the issue in the test suite is resolved (in quite different way, but OK).

rsandell · 2017-03-22T10:04:13Z

🐝

oleg-nenashev · 2017-03-22T10:08:16Z

@reviewbybees done

stephenc · 2017-03-22T21:35:25Z

@jenkinsci/code-reviewers PING

reviewbybees · 2017-03-23T09:17:47Z

This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging.

daniel-beck · 2017-03-23T15:08:13Z

core/src/main/java/hudson/util/CompressedFile.java

@@ -78,22 +76,22 @@ public CompressedFile(File file) {
    /**
     * Gets the OutputStream to write to the file.
     */
-    public OutputStream write() throws FileNotFoundException {
+    public OutputStream write() throws IOException {


Is this safe to do? It may result in unhandled checked exceptions, right?

Well every case I could find was catching the IOException anyway

daniel-beck · 2017-03-23T15:08:13Z

core/src/main/java/hudson/util/io/ReopenableFileOutputStream.java

-            } catch (FileNotFoundException e) {
+                current = Files.newOutputStream(out.toPath(), StandardOpenOption.CREATE,
+                        appendOnNextOpen ? StandardOpenOption.APPEND : StandardOpenOption.TRUNCATE_EXISTING);
+            } catch (FileNotFoundException | NoSuchFileException e) {


Assuming newOutputStream throws the latter, isn't the former obsolete now?

The javadoc for newOutputStream does not mandate which of these should be thrown so safer to catch both

daniel-beck · 2017-03-23T15:08:13Z

core/src/main/java/jenkins/util/xml/XMLUtils.java

@@ -137,16 +139,9 @@ public static void safeTransform(@Nonnull Source source, @Nonnull Result out) th
            throw new IllegalArgumentException(String.format("File %s does not exist or is not a 'normal' file.", file.getAbsolutePath()));
        }

-        FileInputStream fileInputStream = new FileInputStream(file);


Shouldn't there be tons of unused imports removed in this PR?

I was trying to keep the diff minimal... if you want I can optimize imports on the modified files, but that was re-sorting them for me with my current IDE and generating a large diff

IntelliJ highlights (low-lights?) unused imports and makes them trivial to remove.

Still, just wondered, not required IMO.

oleg-nenashev · 2017-03-29T09:40:09Z

@stephenc There is still some unaddressed comments

svanoort · 2017-03-29T20:23:30Z

core/src/main/java/hudson/Main.java

@@ -174,11 +173,8 @@ public static int remotePost(String[] args) throws Exception {
                    con.setFixedLengthStreamingMode((int)tmpFile.length());
                    con.connect();
                    // send the data
-                    FileInputStream in = new FileInputStream(tmpFile);
-                    try {
+                    try (InputStream in = Files.newInputStream(tmpFile.toPath())) {
                        Util.copyStream(in,con.getOutputStream());


We're no longer closing quietly here...

svanoort · 2017-03-29T20:25:26Z

core/src/main/java/hudson/Util.java

@@ -1134,7 +1134,7 @@ public static String xmlEscape(@Nonnull String text) {
     * Creates an empty file.
     */
    public static void touch(@Nonnull File file) throws IOException {
-        new FileOutputStream(file).close();
+        Files.newOutputStream(file.toPath()).close();


@stephenc Why not use Files.createFile() with an exists check?

svanoort

Looks reasonable to me -- there may be opportunities for additional improvements in some of the IO methods, but better to make a small change now than wait for perfection.


        Ensure stream is closed to address issue introduced in Jenkins 2.53 w…

…ith PR: jenkinsci/jenkins#2816


        Test was being overly strict and did not pass against Files.newInputS…

…tream behavior after jenkinsci/jenkins#2816.

[JENKINS-42934] Avoid using new FileInputStream / new FileOutputStream

Loading status checks…

f0cd7ae

oleg-nenashev added the needs-more-reviews label Mar 20, 2017

jglick approved these changes Mar 20, 2017

View changes

[JENKINS-42934] Need to catch a different exception

Loading status checks…

a7fc570

stephenc added 2 commits Mar 20, 2017

[JENKINS-42934] When you specify options you need to include CREATE o…

Loading status checks…

211bb29

…r the file will not be created

[JENKINS-42934] Actually use Java's file locking API to lock the file…

Loading status checks…

218d0a5

… on windows

oleg-nenashev requested changes Mar 21, 2017

View changes

stephenc added 2 commits Mar 21, 2017

[JENKINS-42934] A couple of places where FileNotFoundException may be…

Loading status checks…

e603b10

… replaced by NoSuchFileException by JVM shenanigans

[JENKINS-42934] Some unit tests need side-effects of FileInputStream

Loading status checks…

568772c

oleg-nenashev approved these changes Mar 22, 2017

View changes

daniel-beck reviewed Mar 23, 2017

View changes

daniel-beck approved these changes Mar 29, 2017

View changes

svanoort reviewed Mar 29, 2017

View changes

svanoort approved these changes Mar 29, 2017

View changes

daniel-beck merged commit bde09f7 into jenkinsci:master Mar 30, 2017
2 checks passed

2 checks passed

Jenkins This pull request looks good
Details

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

oleg-nenashev mentioned this pull request Apr 19, 2017

[JENKINS-43531] Plugins may not be expecting InvalidPathException #2849

Merged

mdkf added a commit to mdkf/change-assembly-version-plugin that referenced this pull request Sep 8, 2017

Ensure stream is closed to address issue introduced in Jenkins 2.53 w…

1046227

…ith PR: jenkinsci/jenkins#2816

jenkinsci / jenkins

[JENKINS-42934] Avoid using new FileInputStream / new FileOutputStream #2816

[JENKINS-42934] Avoid using new FileInputStream / new FileOutputStream #2816

stephenc commented Mar 20, 2017

stephenc commented Mar 20, 2017

reviewbybees commented Mar 20, 2017

MarkEWaite commented Mar 20, 2017

jglick left a comment

stephenc commented Mar 20, 2017

stephenc commented Mar 20, 2017

oleg-nenashev left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

oleg-nenashev left a comment

rsandell commented Mar 22, 2017

oleg-nenashev commented Mar 22, 2017

stephenc commented Mar 22, 2017

reviewbybees commented Mar 23, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

oleg-nenashev commented Mar 29, 2017

This comment has been minimized.

This comment has been minimized.

svanoort left a comment

jenkinsci / jenkins

Sponsor jenkinsci/jenkins

Join GitHub today

[JENKINS-42934] Avoid using new FileInputStream / new FileOutputStream #2816

[JENKINS-42934] Avoid using new FileInputStream / new FileOutputStream #2816

Conversation

stephenc commented Mar 20, 2017

stephenc commented Mar 20, 2017

reviewbybees commented Mar 20, 2017

MarkEWaite commented Mar 20, 2017

jglick left a comment

stephenc commented Mar 20, 2017

stephenc commented Mar 20, 2017

oleg-nenashev left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

oleg-nenashev left a comment

rsandell commented Mar 22, 2017

oleg-nenashev commented Mar 22, 2017

stephenc commented Mar 22, 2017

reviewbybees commented Mar 23, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

oleg-nenashev commented Mar 29, 2017

This comment has been minimized.

This comment has been minimized.

svanoort left a comment