Revisiting MayHoldCloseableResource

[Brian Goetz]

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