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

Avoid using new FileInputStream / new FileOutputStream

    Details

    • Type: Improvement
    • Status: Closed (View Workflow)
    • Priority: Minor
    • Resolution: Fixed
    • Component/s: core
    • Labels:
      None
    • Similar Issues:

      Description

      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.

        Attachments

          Issue Links

            Activity

            stephenconnolly Stephen Connolly created issue -
            stephenconnolly Stephen Connolly made changes -
            Field Original Value New Value
            Remote Link This issue links to "PR#2816 (Web Link)" [ 15733 ]
            stephenconnolly Stephen Connolly made changes -
            Remote Link This issue links to "JDK-8080225 (Web Link)" [ 15734 ]
            stephenconnolly Stephen Connolly made changes -
            Status Open [ 1 ] In Progress [ 3 ]
            stephenconnolly Stephen Connolly made changes -
            Status In Progress [ 3 ] In Review [ 10005 ]
            stephenconnolly Stephen Connolly made changes -
            Description See [https://bugs.openjdk.java.net/browse/JDK-8080225]

            Basically, {{FileInputStream}} and {{FileOutputStream}} bothe 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.
            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.
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Stephen Connolly
            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/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
            test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java
            test/src/test/java/hudson/tools/JDKInstallerTest.java
            http://jenkins-ci.org/commit/jenkins/f0cd7ae8ff269dd738e3377a62f3fbebebf9aef6
            Log:
            JENKINS-42934 Avoid using new FileInputStream / new FileOutputStream

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Stephen Connolly 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/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 test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java test/src/test/java/hudson/tools/JDKInstallerTest.java http://jenkins-ci.org/commit/jenkins/f0cd7ae8ff269dd738e3377a62f3fbebebf9aef6 Log: JENKINS-42934 Avoid using new FileInputStream / new FileOutputStream
            Hide
            scm_issue_link 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/a7fc5701584bc28cd34fc3f018cc107616f7cd9a
            Log:
            JENKINS-42934 Need to catch a different exception

            Show
            scm_issue_link 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/a7fc5701584bc28cd34fc3f018cc107616f7cd9a Log: JENKINS-42934 Need to catch a different exception
            Hide
            scm_issue_link 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

            Show
            scm_issue_link 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
            Hide
            scm_issue_link 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

            Show
            scm_issue_link 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
            Hide
            scm_issue_link 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

            Show
            scm_issue_link 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
            Hide
            scm_issue_link 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

            Show
            scm_issue_link 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
            Hide
            scm_issue_link 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

            Show
            scm_issue_link 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
            Hide
            schristou Steven Christou added a comment -

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

            Show
            schristou Steven Christou added a comment - Stephen Connolly  It looks like this was merged. I assume this should be marked as fixed?
            Hide
            oleg_nenashev Oleg Nenashev added a comment -

            released in 2.53

            Show
            oleg_nenashev Oleg Nenashev added a comment - released in 2.53
            oleg_nenashev Oleg Nenashev made changes -
            Status In Review [ 10005 ] Resolved [ 5 ]
            Resolution Fixed [ 1 ]
            jonasatwork Jonas Jonsson made changes -
            Link This issue relates to JENKINS-45057 [ JENKINS-45057 ]
            stephenconnolly Stephen Connolly made changes -
            Link This issue is related to JENKINS-45903 [ JENKINS-45903 ]
            stephenconnolly Stephen Connolly made changes -
            Status Resolved [ 5 ] Closed [ 6 ]
            Hide
            dtrebbien 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?

            Show
            dtrebbien 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?
            Hide
            stephenconnolly 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

            Show
            stephenconnolly 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
            Hide
            dtrebbien 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.

            Show
            dtrebbien 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.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: