Details
-
Improvement
-
Status: Closed (View Workflow)
-
Minor
-
Resolution: Fixed
-
None
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
- is related to
-
JENKINS-45903 transient file handle leak in LargeText.GzipAwareSession.isGzipStream(File)
-
- Resolved
-
- relates to
-
-
- Resolved
-
- links to
Activity
| Field | Original Value | New Value |
|---|---|---|
| Remote Link | This issue links to "PR#2816 (Web Link)" [ 15733 ] |
| Remote Link | This issue links to "JDK-8080225 (Web Link)" [ 15734 ] |
| Status | Open [ 1 ] | In Progress [ 3 ] |
| Status | In Progress [ 3 ] | In Review [ 10005 ] |
| 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. |
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
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
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
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
Steven Christou
added a comment - 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? | Resolution | Fixed [ 1 ] | |
| Status | In Review [ 10005 ] | Resolved [ 5 ] |
Jonas Jonsson
made changes -
| Link |
This issue relates to |
| Link |
This issue is related to |
| Status | Resolved [ 5 ] | Closed [ 6 ] |
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?
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
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.
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.
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-42934Avoid using new FileInputStream / new FileOutputStream