Discussion: easier Stream closing

[Remi Forax]

----- Original Message -----
> From: "Tagir Valeev" <amaembo at gmail.com>
> To: "core-libs-dev" <core-libs-dev at openjdk.java.net>
> Sent: Dimanche 26 Septembre 2021 11:27:58
> Subject: Discussion: easier Stream closing

> Hello!
> 
> With current NIO file API, a very simple problem to get the list of
> all files in the directory requires some ceremony:
> 
> List<Path> paths;
> try (Stream<Path> stream = Files.list(Path.of("/etc"))) {
>    paths = stream.toList();
> }
> 
> If we skip try-with-resources, we may experience OS file handles leak,
> so it's desired to keep it. Yet, it requires doing boring stuff. In
> this sense, classic new File("/etc").list() was somewhat more
> convenient (despite its awful error handling).
> 
> I like how this problem is solved in StackWalker API [1]: it limits
> the lifetime of the Stream by requiring a user-specified function to
> consume it. After the function is applied, the stream is closed
> automatically. We could add a similar overload to the Files API. As an
> additional feature, we could also translate all UncheckedIOException's
> back to IOException for more uniform exception processing:
> 
> /**
> * @param dir  The path to the directory
> * @param function  function to apply to the stream of directory files
> * @param <T> type of the result
> * @return result of the function
> * @throws IOException if an I/O error occurs when opening the directory, or
> * UncheckedIOException is thrown by the supplied function.
> */
> public static <T> T list(Path dir, Function<? super Stream<Path>, ?
> extends T> function) throws IOException {
>    try (Stream<Path> stream = Files.list(dir)) {
>        return function.apply(stream);
>    }
>    catch (UncheckedIOException exception) {
>        throw exception.getCause();
>    }
> }
> 
> In this case, getting the List of all files in the directory will be
> as simple as
> 
> List<Path> paths = Files.list(Path.of("/etc"), Stream::toList);
> This doesn't limit the flexibility. For example, if we need only file
> names instead of full paths, we can do this:
> List<Path> paths = Files.list(Path.of("/etc"), s ->
> s.map(Path::getFileName).toList());


It seems a nice addition, forgetting the try-with-resources is a very common source of bugs in my students code.
Part of the issue is that there is no way to easily test if there are still open descriptors during tests.
I wonder if the VM can not offer such test using a special flag (-XX:+CheckPendingFileDescriptor) or something like that.

> 
> Alternatively, we could enhance the BaseStream interface in a similar
> way. It won't allow us to translate exceptions, but could be useful
> for every stream that must be closed after consumption:
> 
> // in BaseStream:
> 
> /**
> * Apply a given function to this stream, then close the stream.
> * No further operation on the stream will be possible after that.
> *
> * @param function function to apply
> * @param <R> type of the function result
> * @return result of the function
> * @see #close()
> */
> default <R> R consumeAndClose(Function<? super S, ? extends R> function) {
>    try(@SuppressWarnings("unchecked") S s = (S) this) {
>        return function.apply(s);
>    }
> }
> 
> The usage would be a little bit longer but still more pleasant than
> explicit try-with-resources:
> 
> List<Path> list = Files.list(Path.of("/etc")).consumeAndClose(Stream::toList);
> 
> On the other hand, in this case, we are free to put intermediate
> operations outside of consumeAndClose, so we won't need to nest
> everything inside the function. Only terminal operation should be
> placed inside the consumeAndClose. E.g., if we need file names only,
> like above, we can do this:
> 
> List<Path> list =
> Files.list(Path.of("/etc")).map(Path::getFileName).consumeAndClose(Stream::toList);
> 
> What do you think?

This one does not work because if Path::getFileName fails with an exception, close() will not be called.
What you are proposing here is equivalent to
  try(var stream = Files.list(Path.of("/etc")).map(Path::getFileName)) {
    stream.toList()
  }

instead of
  try(var stream = Files.list(Path.of("/etc"))) {
    stream..map(Path::getFileName).toList()
  }

regards,
Rémi

> 
> 
> [1]
> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/StackWalker.html#walk(java.util.function.Function)