Revisiting MayHoldCloseableResource

[Sam Pullara]

Isn't Closeable basically defined this way?

"A Closeable is a source or destination of data that can be closed. The close method is invoked to release resources that the object is holding (such as open files)."

It is interesting to me that Closeable extends AutoCloseable and the latter has a stricter requirements in the Javadocs.

Sam

On Aug 13, 2013, at 10:03 AM, Brian Goetz <brian.goetz at oracle.com> 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.
>>> 
>>>