Revisiting MayHoldCloseableResource

[Kevin Bourrillion]

On Tue, Aug 13, 2013 at 1:03 AM, Joshua Bloch <josh at bloch.us> wrote:

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

Yet, alternatives were heavily discussed and this looks like the viable
solution.

We have to view AutoCloseable as exposing the *ability* to be closed (which
may be a no-op), not as implying a burden to the caller that it *must* be
closed. ByteArrayInputStream etc. already show that we can't effectively
surface that responsibility in the type hierarchy alone.  There needs to be
a separate means for static analyses to judge whether an instance *needed* to
be closed.



>
>      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.
>>>
>>>
>>>
>


-- 
Kevin Bourrillion | Java Librarian | Google, Inc. | kevinb at google.com