[JENKINS-42934] Avoid using new FileInputStream / new FileOutputStream by stephenc · Pull Request #2816 · jenkinsci/jenkins

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

@reviewbybees

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

Also @jenkinsci/code-reviewers

ghost · 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

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)

…r the file will not be created

… 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:36:00Z

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

        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:39:43Z

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

        // 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:43:21Z

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?

… replaced by NoSuchFileException by JVM shenanigans

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

ghost · 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-23T14:14:50Z

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

     * 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:02:15Z

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:04:51Z

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

            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

-                    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

     */
    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.

…ith PR: jenkinsci/jenkins#2816

…tream behavior after jenkinsci/jenkins#2816.

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

f0cd7ae

oleg-nenashev added the needs-more-reviews Complex change, which would benefit from more eyes label Mar 20, 2017

jglick approved these changes Mar 20, 2017

View reviewed changes

[JENKINS-42934] Need to catch a different exception

a7fc570

stephenc added 2 commits March 20, 2017 16:02

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

211bb29

…r the file will not be created

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

218d0a5

… on windows

oleg-nenashev requested changes Mar 21, 2017

View reviewed changes

stephenc added 2 commits March 21, 2017 13:55

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

e603b10

… replaced by NoSuchFileException by JVM shenanigans

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

568772c

oleg-nenashev approved these changes Mar 22, 2017

View reviewed changes

daniel-beck reviewed Mar 23, 2017

View reviewed changes

daniel-beck approved these changes Mar 29, 2017

View reviewed changes

svanoort reviewed Mar 29, 2017

View reviewed changes

svanoort approved these changes Mar 29, 2017

View reviewed changes

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

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

jglick added a commit to jglick/compress-artifacts-plugin that referenced this pull request Mar 5, 2018

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

b720193

…tream behavior after jenkinsci/jenkins#2816.

Uh oh!

Conversation

stephenc commented Mar 20, 2017

Uh oh!

stephenc commented Mar 20, 2017

Uh oh!

ghost commented Mar 20, 2017

Uh oh!

MarkEWaite commented Mar 20, 2017

Uh oh!

jglick left a comment

Choose a reason for hiding this comment

Uh oh!

stephenc commented Mar 20, 2017

Uh oh!

stephenc commented Mar 20, 2017

Uh oh!

oleg-nenashev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev left a comment

Choose a reason for hiding this comment

Uh oh!

rsandell commented Mar 22, 2017

Uh oh!

oleg-nenashev commented Mar 22, 2017

Uh oh!

stephenc commented Mar 22, 2017

Uh oh!

ghost commented Mar 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svanoort left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants