Revisiting MayHoldCloseableResource

[Doug Lea]

On 08/15/2013 04:25 PM, Brian Goetz wrote:
> Last chance for comment.

Sorry for delay. (Swamped...)

My sense is that your AutpCloseable update could still do better in
separating the underlying issue (that an object may hold resources)
from the usage guidance ("must close" or not.) Here's a shot at it:


/**
  * An object that may hold resources (such as file or socket handles)
  * until it is closed. The {@link #close()} method is called
  * automatically when exiting a {@code try}-with-resources block for
  * which the resource has been declared in the header. This
  * construction ensures prompt release of resources, avoiding resource
  * exhaustion exceptions and errors that may otherwise occur.
  *
  * @apiNote
  * <p>It is possible, and in fact common, for a base class to
  * implement AutoCloseable even though not all of its subclasses
  * (perhaps only a few) hold releasable resources. It is best practice
  * for usages applying to <em>any</em> possible subclass to use {@code
  * try}-with-resources constructions, but those applying only to sets
  * of classes that do not hold releasable resources need not do so.
  * In frameworks such as {@link java.util.Stream}, that support both
  * IO-based and non-IO-based forms, it is unnecessary to use {@code
  * try}-with-resources blocks in methods that will never be applied to
  * IO-based forms.
  *
  * @author Josh Bloch
  * @since 1.7
  */
@FunctionalInterface
public interface AutoCloseable {


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