Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-42934

Avoid using new FileInputStream / new FileOutputStream

    • Icon: Improvement Improvement
    • Resolution: Fixed
    • Icon: Minor Minor
    • core
    • None

      See https://bugs.openjdk.java.net/browse/JDK-8080225

      Basically, FileInputStream and FileOutputStream both use a finalizer to ensure that the resources are closed.

      Even with programmers do call close() after using FileInputStream, its finalize() method will still be called. In other words, still get the side effect of the FinalReference being registered at FileInputStream allocation time, and also reference processing to reclaim the FinalReference during GC (any GC solution has to deal with this).

      The recommended solution is to switch to Files.newInputStream and Files.newOutputStream respectively as these use a reference queue to clean up unclosed streams that are eligible for GC rather than waiting for a finalizer always.

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

          Code changed in jenkins
          User: Stephen Connolly
          Path:
          core/src/main/java/hudson/util/io/RewindableFileOutputStream.java
          http://jenkins-ci.org/commit/jenkins/211bb293381802f7c653c6e6cee965ab316d0fc4
          Log:
          JENKINS-42934 When you specify options you need to include CREATE or the file will not be created

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Stephen Connolly Path: core/src/main/java/hudson/util/io/RewindableFileOutputStream.java http://jenkins-ci.org/commit/jenkins/211bb293381802f7c653c6e6cee965ab316d0fc4 Log: JENKINS-42934 When you specify options you need to include CREATE or the file will not be created

          Code changed in jenkins
          User: Stephen Connolly
          Path:
          core/src/test/java/hudson/UtilTest.java
          http://jenkins-ci.org/commit/jenkins/218d0a55169aa030646e4f7a0469e9ce8fe2c93f
          Log:
          JENKINS-42934 Actually use Java's file locking API to lock the file on windows

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Stephen Connolly Path: core/src/test/java/hudson/UtilTest.java http://jenkins-ci.org/commit/jenkins/218d0a55169aa030646e4f7a0469e9ce8fe2c93f Log: JENKINS-42934 Actually use Java's file locking API to lock the file on windows

          Code changed in jenkins
          User: Stephen Connolly
          Path:
          core/src/main/java/hudson/cli/BuildCommand.java
          core/src/main/java/hudson/util/io/ReopenableFileOutputStream.java
          core/src/test/java/jenkins/util/VirtualFileTest.java
          http://jenkins-ci.org/commit/jenkins/e603b100889efb71cc71949dc9df7d8eeae5256a
          Log:
          JENKINS-42934 A couple of places where FileNotFoundException may be replaced by NoSuchFileException by JVM shenanigans

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Stephen Connolly Path: core/src/main/java/hudson/cli/BuildCommand.java core/src/main/java/hudson/util/io/ReopenableFileOutputStream.java core/src/test/java/jenkins/util/VirtualFileTest.java http://jenkins-ci.org/commit/jenkins/e603b100889efb71cc71949dc9df7d8eeae5256a Log: JENKINS-42934 A couple of places where FileNotFoundException may be replaced by NoSuchFileException by JVM shenanigans

          Code changed in jenkins
          User: Stephen Connolly
          Path:
          core/src/test/java/hudson/UtilTest.java
          http://jenkins-ci.org/commit/jenkins/568772cddc78f76e8eb395f4a5c39f397e0c1935
          Log:
          JENKINS-42934 Some unit tests need side-effects of FileInputStream

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Stephen Connolly Path: core/src/test/java/hudson/UtilTest.java http://jenkins-ci.org/commit/jenkins/568772cddc78f76e8eb395f4a5c39f397e0c1935 Log: JENKINS-42934 Some unit tests need side-effects of FileInputStream

          Code changed in jenkins
          User: Daniel Beck
          Path:
          cli/src/main/java/hudson/cli/PrivateKeyProvider.java
          core/src/main/java/hudson/ClassicPluginStrategy.java
          core/src/main/java/hudson/FilePath.java
          core/src/main/java/hudson/FileSystemProvisioner.java
          core/src/main/java/hudson/Main.java
          core/src/main/java/hudson/PluginWrapper.java
          core/src/main/java/hudson/Util.java
          core/src/main/java/hudson/WebAppMain.java
          core/src/main/java/hudson/XmlFile.java
          core/src/main/java/hudson/cli/BuildCommand.java
          core/src/main/java/hudson/lifecycle/WindowsInstallerLink.java
          core/src/main/java/hudson/model/FileParameterValue.java
          core/src/main/java/hudson/model/Queue.java
          core/src/main/java/hudson/model/Run.java
          core/src/main/java/hudson/model/UpdateCenter.java
          core/src/main/java/hudson/tools/JDKInstaller.java
          core/src/main/java/hudson/util/AtomicFileWriter.java
          core/src/main/java/hudson/util/CompressedFile.java
          core/src/main/java/hudson/util/IOUtils.java
          core/src/main/java/hudson/util/SecretRewriter.java
          core/src/main/java/hudson/util/StreamTaskListener.java
          core/src/main/java/hudson/util/TextFile.java
          core/src/main/java/hudson/util/io/ReopenableFileOutputStream.java
          core/src/main/java/hudson/util/io/RewindableFileOutputStream.java
          core/src/main/java/hudson/util/io/TarArchiver.java
          core/src/main/java/hudson/util/io/ZipArchiver.java
          core/src/main/java/jenkins/diagnosis/HsErrPidList.java
          core/src/main/java/jenkins/security/DefaultConfidentialStore.java
          core/src/main/java/jenkins/util/AntClassLoader.java
          core/src/main/java/jenkins/util/JSONSignatureValidator.java
          core/src/main/java/jenkins/util/VirtualFile.java
          core/src/main/java/jenkins/util/io/FileBoolean.java
          core/src/main/java/jenkins/util/xml/XMLUtils.java
          core/src/test/java/hudson/FilePathTest.java
          core/src/test/java/hudson/PluginManagerTest.java
          core/src/test/java/hudson/UtilTest.java
          core/src/test/java/hudson/model/LoadStatisticsTest.java
          core/src/test/java/hudson/os/SUTester.java
          core/src/test/java/hudson/util/io/TarArchiverTest.java
          core/src/test/java/hudson/util/io/ZipArchiverTest.java
          core/src/test/java/jenkins/util/VirtualFileTest.java
          test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java
          test/src/test/java/hudson/tools/JDKInstallerTest.java
          http://jenkins-ci.org/commit/jenkins/bde09f70afaf10d5e1453c257058a56b07556e8e
          Log:
          Merge pull request #2816 from stephenc/jenkins-42934

          JENKINS-42934 Avoid using new FileInputStream / new FileOutputStream

          Compare: https://github.com/jenkinsci/jenkins/compare/0ddf2d5be770...bde09f70afaf

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Daniel Beck Path: cli/src/main/java/hudson/cli/PrivateKeyProvider.java core/src/main/java/hudson/ClassicPluginStrategy.java core/src/main/java/hudson/FilePath.java core/src/main/java/hudson/FileSystemProvisioner.java core/src/main/java/hudson/Main.java core/src/main/java/hudson/PluginWrapper.java core/src/main/java/hudson/Util.java core/src/main/java/hudson/WebAppMain.java core/src/main/java/hudson/XmlFile.java core/src/main/java/hudson/cli/BuildCommand.java core/src/main/java/hudson/lifecycle/WindowsInstallerLink.java core/src/main/java/hudson/model/FileParameterValue.java core/src/main/java/hudson/model/Queue.java core/src/main/java/hudson/model/Run.java core/src/main/java/hudson/model/UpdateCenter.java core/src/main/java/hudson/tools/JDKInstaller.java core/src/main/java/hudson/util/AtomicFileWriter.java core/src/main/java/hudson/util/CompressedFile.java core/src/main/java/hudson/util/IOUtils.java core/src/main/java/hudson/util/SecretRewriter.java core/src/main/java/hudson/util/StreamTaskListener.java core/src/main/java/hudson/util/TextFile.java core/src/main/java/hudson/util/io/ReopenableFileOutputStream.java core/src/main/java/hudson/util/io/RewindableFileOutputStream.java core/src/main/java/hudson/util/io/TarArchiver.java core/src/main/java/hudson/util/io/ZipArchiver.java core/src/main/java/jenkins/diagnosis/HsErrPidList.java core/src/main/java/jenkins/security/DefaultConfidentialStore.java core/src/main/java/jenkins/util/AntClassLoader.java core/src/main/java/jenkins/util/JSONSignatureValidator.java core/src/main/java/jenkins/util/VirtualFile.java core/src/main/java/jenkins/util/io/FileBoolean.java core/src/main/java/jenkins/util/xml/XMLUtils.java core/src/test/java/hudson/FilePathTest.java core/src/test/java/hudson/PluginManagerTest.java core/src/test/java/hudson/UtilTest.java core/src/test/java/hudson/model/LoadStatisticsTest.java core/src/test/java/hudson/os/SUTester.java core/src/test/java/hudson/util/io/TarArchiverTest.java core/src/test/java/hudson/util/io/ZipArchiverTest.java core/src/test/java/jenkins/util/VirtualFileTest.java test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java test/src/test/java/hudson/tools/JDKInstallerTest.java http://jenkins-ci.org/commit/jenkins/bde09f70afaf10d5e1453c257058a56b07556e8e Log: Merge pull request #2816 from stephenc/jenkins-42934 JENKINS-42934 Avoid using new FileInputStream / new FileOutputStream Compare: https://github.com/jenkinsci/jenkins/compare/0ddf2d5be770...bde09f70afaf

          stephenconnolly It looks like this was merged. I assume this should be marked as fixed?

          Steven Christou added a comment - stephenconnolly  It looks like this was merged. I assume this should be marked as fixed?

          Oleg Nenashev added a comment -

          released in 2.53

          Oleg Nenashev added a comment - released in 2.53

          Regarding this paragraph:

          The recommended solution is to switch to Files.newInputStream and Files.newOutputStream respectively as these use a reference queue to clean up unclosed streams that are eligible for GC rather than waiting for a finalizer always.

          I tried looking through the OpenJDK source code, but I am not seeing where this reference queue is. It looks like one cannot rely on an unclosed stream created by Files.newInputStream, etc. being closed automatically.

          java.nio.file.Files has this:

              private static FileSystemProvider provider(Path path) {
                  return path.getFileSystem().provider();
              }
          
              //...
          
              public static InputStream newInputStream(Path path, OpenOption... options)
                  throws IOException
              {
                  return provider(path).newInputStream(path, options);
              }
          
              //...
          
              public static SeekableByteChannel newByteChannel(Path path,
                                                               Set<? extends OpenOption> options,
                                                               FileAttribute<?>... attrs)
                  throws IOException
              {
                  return provider(path).newByteChannel(path, options, attrs);
              }
          
              //...
          
              public static SeekableByteChannel newByteChannel(Path path, OpenOption... options)
                  throws IOException
              {
                  Set<OpenOption> set = new HashSet<OpenOption>(options.length);
                  Collections.addAll(set, options);
                  return newByteChannel(path, set);
              }
          

          java.nio.file.spi.FileSystemProvider has this:

              public InputStream newInputStream(Path path, OpenOption... options)
                  throws IOException
              {
                  if (options.length > 0) {
                      for (OpenOption opt: options) {
                          // All OpenOption values except for APPEND and WRITE are allowed
                          if (opt == StandardOpenOption.APPEND ||
                              opt == StandardOpenOption.WRITE)
                              throw new UnsupportedOperationException("'" + opt + "' not allowed");
                      }
                  }
                  return Channels.newInputStream(Files.newByteChannel(path, options));
              }
          

          sun.nio.fs.UnixFileSystemProvider has this:

              @Override
              public SeekableByteChannel newByteChannel(Path obj,
                                                        Set<? extends OpenOption> options,
                                                        FileAttribute<?>... attrs)
                   throws IOException
              {
                  UnixPath file = UnixPath.toUnixPath(obj);
                  int mode = UnixFileModeAttribute
                      .toUnixMode(UnixFileModeAttribute.ALL_READWRITE, attrs);
                  try {
                      return UnixChannelFactory.newFileChannel(file, options, mode);
                  } catch (UnixException x) {
                      x.rethrowAsIOException(file);
                      return null;  // keep compiler happy
                  }
              }
          

          sun.nio.fs.UnixChannelFactory has this:

              static FileChannel newFileChannel(int dfd,
                                                UnixPath path,
                                                String pathForPermissionCheck,
                                                Set<? extends OpenOption> options,
                                                int mode)
                  throws UnixException
              {
                  Flags flags = Flags.toFlags(options);
          
                  // default is reading; append => writing
                  if (!flags.read && !flags.write) {
                      if (flags.append) {
                          flags.write = true;
                      } else {
                          flags.read = true;
                      }
                  }
          
                  // validation
                  if (flags.read && flags.append)
                      throw new IllegalArgumentException("READ + APPEND not allowed");
                  if (flags.append && flags.truncateExisting)
                      throw new IllegalArgumentException("APPEND + TRUNCATE_EXISTING not allowed");
          
                  FileDescriptor fdObj = open(dfd, path, pathForPermissionCheck, flags, mode);
                  return FileChannelImpl.open(fdObj, flags.read, flags.write, flags.append, null);
              }
          
              //...
          
              static FileChannel newFileChannel(UnixPath path,
                                                Set<? extends OpenOption> options,
                                                int mode)
                  throws UnixException
              {
                  return newFileChannel(-1, path, null, options, mode);
              }
          

          sun.nio.ch.FileChannelImpl has this:

          package sun.nio.ch;
          
          import java.io.FileDescriptor;
          import java.io.IOException;
          import java.nio.ByteBuffer;
          import java.nio.MappedByteBuffer;
          import java.nio.channels.*;
          import java.util.ArrayList;
          import java.util.List;
          import java.security.AccessController;
          import sun.misc.Cleaner;
          import sun.security.action.GetPropertyAction;
          
          public class FileChannelImpl
              extends FileChannel
          {
              //...
          
              private FileChannelImpl(FileDescriptor fd, boolean readable,
                                      boolean writable, boolean append, Object parent)
              {
                  this.fd = fd;
                  this.readable = readable;
                  this.writable = writable;
                  this.append = append;
                  this.parent = parent;
                  this.nd = new FileDispatcherImpl(append);
              }
          
              //...
          
              public static FileChannel open(FileDescriptor fd,
                                             boolean readable, boolean writable,
                                             boolean append, Object parent)
              {
                  return new FileChannelImpl(fd, readable, writable, append, parent);
              }
          
              //...
          
              protected void implCloseChannel() throws IOException {
                  // Release and invalidate any locks that we still hold
                  if (fileLockTable != null) {
                      for (FileLock fl: fileLockTable.removeAll()) {
                          synchronized (fl) {
                              if (fl.isValid()) {
                                  nd.release(fd, fl.position(), fl.size());
                                  ((FileLockImpl)fl).invalidate();
                              }
                          }
                      }
                  }
          
                  nd.preClose(fd);
                  threads.signalAndWait();
          
                  if (parent != null) {
          
                      // Close the fd via the parent stream's close method.  The parent
                      // will reinvoke our close method, which is defined in the
                      // superclass AbstractInterruptibleChannel, but the isOpen logic in
                      // that method will prevent this method from being reinvoked.
                      //
                      ((java.io.Closeable)parent).close();
                  } else {
                      nd.close(fd);
                  }
          
              }
          
          //...
          

          It looks to me like the FileDescriptor is not closed unless close() (defined by java.nio.channels.spi.AbstractInterruptibleChannel, which calls implCloseChannel() to do the actual closing) is called.

          What am I missing?

          Daniel Trebbien added a comment - Regarding this paragraph: The recommended solution is to switch to Files.newInputStream and Files.newOutputStream respectively as these use a reference queue to clean up unclosed streams that are eligible for GC rather than waiting for a finalizer always. I tried looking through the OpenJDK source code, but I am not seeing where this reference queue is. It looks like one cannot rely on an unclosed stream created by Files.newInputStream , etc. being closed automatically. java.nio.file.Files has this: private static FileSystemProvider provider(Path path) { return path.getFileSystem().provider(); } //... public static InputStream newInputStream(Path path, OpenOption... options) throws IOException { return provider(path).newInputStream(path, options); } //... public static SeekableByteChannel newByteChannel(Path path, Set<? extends OpenOption> options, FileAttribute<?>... attrs) throws IOException { return provider(path).newByteChannel(path, options, attrs); } //... public static SeekableByteChannel newByteChannel(Path path, OpenOption... options) throws IOException { Set<OpenOption> set = new HashSet<OpenOption>(options.length); Collections.addAll(set, options); return newByteChannel(path, set); } java.nio.file.spi.FileSystemProvider has this: public InputStream newInputStream(Path path, OpenOption... options) throws IOException { if (options.length > 0) { for (OpenOption opt: options) { // All OpenOption values except for APPEND and WRITE are allowed if (opt == StandardOpenOption.APPEND || opt == StandardOpenOption.WRITE) throw new UnsupportedOperationException( " '" + opt + "' not allowed" ); } } return Channels.newInputStream(Files.newByteChannel(path, options)); } sun.nio.fs.UnixFileSystemProvider has this: @Override public SeekableByteChannel newByteChannel(Path obj, Set<? extends OpenOption> options, FileAttribute<?>... attrs) throws IOException { UnixPath file = UnixPath.toUnixPath(obj); int mode = UnixFileModeAttribute .toUnixMode(UnixFileModeAttribute.ALL_READWRITE, attrs); try { return UnixChannelFactory.newFileChannel(file, options, mode); } catch (UnixException x) { x.rethrowAsIOException(file); return null ; // keep compiler happy } } sun.nio.fs.UnixChannelFactory has this: static FileChannel newFileChannel( int dfd, UnixPath path, String pathForPermissionCheck, Set<? extends OpenOption> options, int mode) throws UnixException { Flags flags = Flags.toFlags(options); // default is reading; append => writing if (!flags.read && !flags.write) { if (flags.append) { flags.write = true ; } else { flags.read = true ; } } // validation if (flags.read && flags.append) throw new IllegalArgumentException( "READ + APPEND not allowed" ); if (flags.append && flags.truncateExisting) throw new IllegalArgumentException( "APPEND + TRUNCATE_EXISTING not allowed" ); FileDescriptor fdObj = open(dfd, path, pathForPermissionCheck, flags, mode); return FileChannelImpl.open(fdObj, flags.read, flags.write, flags.append, null ); } //... static FileChannel newFileChannel(UnixPath path, Set<? extends OpenOption> options, int mode) throws UnixException { return newFileChannel(-1, path, null , options, mode); } sun.nio.ch.FileChannelImpl has this: package sun.nio.ch; import java.io.FileDescriptor; import java.io.IOException; import java.nio.ByteBuffer; import java.nio.MappedByteBuffer; import java.nio.channels.*; import java.util.ArrayList; import java.util.List; import java.security.AccessController; import sun.misc.Cleaner; import sun.security.action.GetPropertyAction; public class FileChannelImpl extends FileChannel { //... private FileChannelImpl(FileDescriptor fd, boolean readable, boolean writable, boolean append, Object parent) { this .fd = fd; this .readable = readable; this .writable = writable; this .append = append; this .parent = parent; this .nd = new FileDispatcherImpl(append); } //... public static FileChannel open(FileDescriptor fd, boolean readable, boolean writable, boolean append, Object parent) { return new FileChannelImpl(fd, readable, writable, append, parent); } //... protected void implCloseChannel() throws IOException { // Release and invalidate any locks that we still hold if (fileLockTable != null ) { for (FileLock fl: fileLockTable.removeAll()) { synchronized (fl) { if (fl.isValid()) { nd.release(fd, fl.position(), fl.size()); ((FileLockImpl)fl).invalidate(); } } } } nd.preClose(fd); threads.signalAndWait(); if (parent != null ) { // Close the fd via the parent stream's close method. The parent // will reinvoke our close method, which is defined in the // superclass AbstractInterruptibleChannel, but the isOpen logic in // that method will prevent this method from being reinvoked. // ((java.io.Closeable)parent).close(); } else { nd.close(fd); } } //... It looks to me like the FileDescriptor is not closed unless close() (defined by java.nio.channels.spi.AbstractInterruptibleChannel , which calls implCloseChannel() to do the actual closing) is called. What am I missing?

          So the issue with `FileInputStream` and `FileOutputStream` is that they do not release the file handle until finalization even if you close them.

          What we have done here is ensure that almost all cases will close the stream immediately (there are some cases where we have to pass the stream on to another method / thread and hence we could still leak if analysis of the transfer of ownership is incorrect) thus the switch should fix the case of dangling unclosed file handles.

          The reports I saw from Hadoop indicated that these new methods used a reference queue to perform tidy up rather than relying on a finalizer... if that proves to be a rumour then so be it, and we would have to assume that leaks from the new code paths should therefore be even easier to catch (as they should accumulate faster) and therefore fixing should be easier too

          Stephen Connolly added a comment - So the issue with `FileInputStream` and `FileOutputStream` is that they do not release the file handle until finalization even if you close them. What we have done here is ensure that almost all cases will close the stream immediately (there are some cases where we have to pass the stream on to another method / thread and hence we could still leak if analysis of the transfer of ownership is incorrect) thus the switch should fix the case of dangling unclosed file handles. The reports I saw from Hadoop indicated that these new methods used a reference queue to perform tidy up rather than relying on a finalizer... if that proves to be a rumour then so be it, and we would have to assume that leaks from the new code paths should therefore be even easier to catch (as they should accumulate faster) and therefore fixing should be easier too

          There might be an issue with using Files.newOutputStream() in hudson.util.StreamTaskListener(File out, Charset charset) because the operating system file handle might not be closed.

          For example, hudson.model.TaskThread.ListenerAndText.forFile(File f, TaskAction context) creates a StreamTaskListener for the file, but there is no way to close the StreamTaskListener.

          I did a "Find Usages" in NetBeans for the ListenerAndText.forFile() APIs, and it appears that they are not being used.

          Daniel Trebbien added a comment - There might be an issue with using Files.newOutputStream() in hudson.util.StreamTaskListener(File out, Charset charset) because the operating system file handle might not be closed. For example, hudson.model.TaskThread.ListenerAndText.forFile(File f, TaskAction context) creates a StreamTaskListener for the file, but there is no way to close the StreamTaskListener . I did a "Find Usages" in NetBeans for the ListenerAndText.forFile() APIs, and it appears that they are not being used.

            stephenconnolly Stephen Connolly
            stephenconnolly Stephen Connolly
            Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

              Created:
              Updated:
              Resolved: