Avoid FileInputStream and FileOutputStream

[leventov]

Since they override finalize() in OpenJDK, I think it's better to avoid instantiating those classes in FileByteSource and FileByteSink, instead using Files.newInputStream and newOutputStream.

[cgdecker]

I'm not sure that it's worth doing this right now since it would require creating a diff between the JRE/Android flavors, since Android didn't support java.nio.file at all until API level 26 (Android O).

[cpovirk]

Ooo, I hadn't thought about Android. That raises an interesting point, which is that the code diff between flavors would produce a behavior diff. And of course it's also a behavior diff versus previous versions of Guava. That's the point, after all :) But it could also mean that someone updates a project's version of Guava (or builds against guava-android and then runs against guava-jre) and suddenly starts running out of file descriptors. Of course the code is buggy, but we'd want to have a very strong reason for taking risks with it. Presumably this fear is why the JDK itself hasn't (AFAIK) removed finalize() from FileInputStream and FileOutputStream themselves, even as it's omitted them from the streams returned from the new methods on Files.

We're presumably not going to introduce new methods just to avoid this. We could maybe introduce new package-private methods that could be used by ByteSource and friends themselves when they're sure they're going to close the streams (so there's no concern about file descriptors), but that would require even more code :)

One thing that we could consider doing now is overriding FileInputStream.finalize() to log an exception before it closes. That would give users with buggy code an opportunity to fix problems before we maybe someday move to the Files methods and cause more subtle problems. (It might also break some tests or something if those tests check for unexpected log messages, but I would feel better about that than about making people run out of file descriptors.)

Or we can just reason that the cost of a finalizer isn't too large in comparison to the cost of I/O, and we could live with the current implementation. Maybe the JDK will someday remove the finalize() implementations. I suppose we could even insert the logging, even if we expect to keep finalization forever.

(Or, for all I know, maybe the Files methods still auto-close somehow, just with a PhantomReference or something.)

[leventov]

Presumably this fear is why the JDK itself hasn't (AFAIK) removed finalize() from FileInputStream and FileOutputStream themselves, even as it's omitted them from the streams returned from the new methods on Files.

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

We're presumably not going to introduce new methods just to avoid this. We could maybe introduce new package-private methods that could be used by ByteSource and friends themselves when they're sure they're going to close the streams

Guava itself could just prefer to use MoreFiles.asByteSource() and asByteSink().

One thing that we could consider doing now is overriding FileInputStream.finalize()

This is probably not a great idea, because FileInputStream.finalize() is already annotated as @Deprecated(forRemoval=true)

[cpovirk]

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

Oh, nice, thanks. I should have opened that Java 10 src.zip that I just downloaded the other day... or at least read the docs :) Although the docs are pretty confusing: "Ensures that the close() method of this file input stream is called when there are no more references to it. The finalize() method does not call close() directly."

Translation: They still kind of have finalization -- but only for instance of subclasses of the stream classes that override close().

And in fact it's still implemented with finalize() -- but not stream.finalize(), which is a complete no-op. The implementation is now in a separate object, which both references the stream and is referenced from it.

So it sounds like we might just be able to let users move to JDK 10, which will remove finalization in the normal case (i.e., the no-subclassing case, the case Guava is in). Maybe it's better for users to confront that all at once when they upgrade Java, rather than split it across Java and Guava upgrades?

Guava itself could just prefer to use MoreFiles.asByteSource() and asByteSink().

True, as can users. I was thinking of the users who are still making calls to Guava like Files.asByteSource(...).read(): We could safely switch such operations to Files.newInputStream() because read() is guaranteed to close the stream, even if we aren't willing to switch Files.asByteSource(...).openStream(), knowing that users might not close it.

This is probably not a great idea, because FileInputStream.finalize() is already annotated as @Deprecated(forRemoval=true)

Thanks.

[cpovirk]

The finalize overrides got removed entirely sometime before Java 21.

There may well still be other advantages to Files.newInputStream aside from finalization, but that removal at least further discourages us from doing this thing that we've already not been doing.