Revisiting MayHoldCloseableResource

[Brian Goetz]

Last chance for comment.

On 8/13/2013 1:03 PM, Brian Goetz wrote:
> OK, here's proposed spec:
>
> For AC:
>
>   /**
> - * A resource that must be closed when it is no longer needed.
> + * A resource that may need to be closed when it is no longer needed,
> and whose
> + * {@link #close()} method will be called automatically when exiting a
> + * {@code try}-with-resources block for which the resource has been
> declared in
> + * the header.  A resource would need to be explicitly closed if it
> holds a
> + * scarce system resource that is not promptly reclaimed by garbage
> collection,
> + * such as a file or socket handle.
> + *
> + * @apiNote
> + * <p>It should generally be possible to determine statically whether a
> resource
> + * that may need to be closed actually does need to be closed (in which
> case one
> + * should consider using try-with-resources to ensure its timely closure).
> + * For example, most {@link InputStream} implementations need to be
> closed,
> + * though {@link ByteArrayInputStream} does not need to be; most {@link
> Stream}
> + * implementations do <em>not</em> need to be closed, but streams which
> encapsulate
> + * IO resources, like those produced by the static factories in {@link
> Files}
> + * such as {@link Files#walk(Path, FileVisitOption...)}, do need to be
> closed.
>
> For BaseStream and friends:
>
> + * <p>Streams have a {@link #close()} method and implement {@link
> AutoCloseable},
> + * but nearly all stream instances do not actually need to be closed
> after use.
> + * Generally, only streams whose source is an IO channel (such as those
> returned
> + * by {@link Files#lines(Path, Charset)}) will require closing.  Most
> streams
> + * are backed by collections, arrays, or generating functions, which
> require no
> + * special resource management.  (If a stream does require closing, it
> can be
> + * declared as a resource in a {@code try}-with-resources statement.)
> + *
>
> For Files.walk and friends:
>
> +     * <p>The returned stream encapsulates one or more {@link
> DirectoryStream}s.
> +     * If timely disposal of file system resources is required, the
> +     * {@code try}-with-resources construct should be used to ensure
> that the
> +     * stream's {@link Stream#close close} method is invoked after the
> stream
> +     * operations are completed.
>
>
> On 8/12/2013 1:43 PM, Brian Goetz 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.
>>>
>>>