Revisiting MayHoldCloseableResource

[Joshua Bloch]

That's exactly what I was thinking. The spec for AutoCloseable is not very
wrong!  I really did intend if for tobjects hings that (almost certainly)
need to be closed. ByteArray{In,Out}put Stream are outliers, and (to some
extent) historical anomalies. They already extended {In,Out}putStream,
which extended Closeable before AutoCloseable was written. And yes, if you
took a delegating IO stream (such as Buffered{In,Out}putStream), you could
generate another Closeable that didn't really need to be closed.  But these
were still outliers. To a large extent, Closeable did what it was supposed
to. If you have Stream extend Closeable, you're opening the floodgates of
Closeables that don't really need to be closed.

Josh


On Sat, Aug 3, 2013 at 7:54 AM, Joe Bowbeer <joe.bowbeer at gmail.com> wrote:

> Brian,
>
> As you wrote, BufferedReader.lines() gets this right and I'm thankful
> enough for that:
>
>         try (BufferedReader r = Files.newBufferedReader(path,
> Charset.defaultCharset())) {
>             anagrams(r.lines()).forEach(System.out::println);
>         }
>
> There's a similar story with DirectoryStream versus Files.list(), I assume.
>
> Why aren't there similar formulations for Files.walk and Files.find?
>
> I'm not convinced that Stream itself should have a close() method; and if
> that is troubling to Files.walk, then it can take a hike.
>
> Fixing the AC spec seems fine, but not relevant to Stream.
>
> --Joe
>
>
> On Sat, Aug 3, 2013 at 3:10 AM, Andrey Breslav <
> andrey.breslav at jetbrains.com> wrote:
>
>> In case you are asking for opinions, I think
>> * we'd better fix the AC spec early than late;
>> * have streams implement (fixed) AC (Option 1) looks better to me than
>> just have close() (Option 2) or not having the Files.* methods (Option 3).
>>
>> --
>> Andrey Breslav
>> http://jetbrains.com
>> Develop with pleasure!
>>
>>
>> On Aug 3, 2013, at 01:39 , Brian Goetz wrote:
>>
>> > There seems to be consensus that the current strawman for
>> MayHoldCloseableResoruce and @HoldsResource is too flawed to proceed.
>> >
>> > Flaw #1 relates to @HoldsResource.  While there seems to be consensus
>> that annotations are reasonable for imparting information like this which
>> is suited to static analysis tools, the current proposal (a) confusingly
>> leaks through into subclass Javadoc and (b) seems too weak.  Conclusion:
>> just dropping @HR from the current scheme (leaving only MHCR <: AC) is an
>> improvement; we can revisit what the correct annotations for hinting at
>> resourceful-ness later.
>> >
>> > Flaw #2 relates to MHCR.  The real problem is that AutoCloseable is
>> overspecified; it says "a resource that must be closed."  This is an
>> overreach; what j.l.AC should say is "has a close method which is called
>> automatically on exit from a TWR block."  Because of this overspecify,
>> subclassing doesn't help, because if an AC must be closed, so must a MHCR.
>>  Conclusion: dropping MHCR and just having BaseStream <: AC is an
>> improvement.
>> >
>> > Flaw #3 relates to the spec of AC.  The conclusion of the core library
>> maintainers is that it is acceptable to correct the spec, and replace the
>> "must" with "may".  This leaves us with a fixed AC, and BaseStream <: AC.
>>  This seems strictly better than where we are now.  Let's call this "Option
>> 1."
>> >
>> > But, it is possible we might still choose to punt further for now.
>> >
>> > Observation: the need for Stream to have a close() method comes about
>> because of the desire to have static factory methods that will create
>> streams that encapsulate a resource that requires closing, when there is no
>> way to get access to that underlying resource.  Such as Files.walk(dir) or
>> Files.lines(path).
>> >
>> > For other cases, like BufferedReader.lines(), the BR itself is AC, so
>> users can handle resource management like:
>> >
>> >  try (BufferedReader b = ...) {
>> >     b.lines()...
>> >  }
>> >
>> > and there is no need to close the *stream* at all, since the user has
>> access to the actual closeable resource.  It is only the static methods
>> such as Files.list(dir), which don't provide the user access in this way.
>>  The methods we provide are:
>> >
>> > Files.list(Path)
>> > Files.walk(Path, maxDepth, options)
>> > Files.walk(Path, options)
>> > Files.find(Path, maxDepth, BiPredicate<Path, BasicFileAttributes>,
>> options)
>> > Files.lines(Path)
>> >
>> > Colin @ Google pointed out that if the spliterators for these streams
>> were lazy (didn't open the resource until the terminal began), and all
>> terminal ops called the close hook in a finally block after processing,
>> then we wouldn't need to be AC, since there would be no way to leak a
>> closeable resource.  (This would move resource-not-found exceptions from
>> the factory method to the terminal op.)
>> >
>> > Still, having a close() method and *not* being AC may look weird to the
>> users.
>> >
>> > Let's call this "Option 2": have a close() method, don't extend AC,
>> make sure that all terminals call close, make sure that all static
>> resource-holding stream factories produce late-binding spliterators.
>> >
>> > Option 3 would be to simply get rid of these methods (note that
>> Files.list and Files.lines are pretty simple wrappers around existing AC
>> abstractions, DirectoryStream and BufferedReader.)
>> >
>> > Orthogonal to Options 2 and 3 would be fixing the spec of AC.
>> >
>> >
>>
>>
>