Revisiting MayHoldCloseableResource

[Joshua Bloch]

Having BaseStream implement AutoCloseable because some streams need to be
closed seems like a bad idea.

     Josh


On Mon, Aug 12, 2013 at 10:43 AM, Brian Goetz <brian.goetz at oracle.com>wrote:

> After discussion both in the EG and within Oracle, here's where I think
> this should land:
>
>  - Eliminate MayHoldCloseableResource, which is fundamentally flawed
> unless the spec for AutoCloseable is fixed, in which case it becomes
> unnecessary.
>  - Fix the spec for AutoCloseable.
>  - Eliminate @HoldsResource, not because it is intrinsically bad, but
> because it is a half solution and we should work towards a complete one for
> 9, likely using something like JSR-308 checkers.  The Eclipse inspector can
> be adjusted to work with the new APIs.
>  - Make BaseStream implement AutoCloseable, with additional verbiage
> regarding closing.
>  - Add additional spec to Files.{walk,lines,etc}, and to combinators like
> concat, regarding their close behavior.
>
> The thing that pushed me over the edge from the various "work around the
> problem" solutions is the existence of flatMap:
>
>   Files.walk(dir)
>        .flatMap(Files::lines)
>        ...
>
> where each entry resulting from walk() will turn into a stream creation
> and traversal within the flatMap implementation, and flatMap itself needs a
> way to ensure it is deterministically closed.  This pushes strongly for
> having a close() method on Stream, which, if you're going to have that, you
> might as well be AutoCloseable to make life easier for users.
>
> The sole challenge here is to keep people from thinking that they *need*
> to close all streams, and therefore mangle their code in bad ways to
> satisfy this perceived need.
>
>
>
> On 8/2/2013 7:39 PM, 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.
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/lambda-libs-spec-experts/attachments/20130813/ad385aaa/attachment.html