Revisiting MayHoldCloseableResource

[Zhong Yu]

On Tue, Aug 13, 2013 at 12:24 PM, Kevin Bourrillion <kevinb at google.com> wrote:
> 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

Now the burden is on the programmer who must reason whether a stream
needs to be closed, with some other information not conveyed by the
type of the object.

> closed. ByteArrayInputStream etc. already show that we can't effectively

ByteArrayInputStream is a singular example that has been over played
to argue against the general semantics of AutoCloseable.

And it is probably a mistake to spec that its close() must have no
effect. That spec is unnecessary and harmful. The close() could gain a
side effect, e.g. dereference the byte array; and a subclass may wish
to attach additional effects to close().

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

I think some destructor like utility is probably the cleanest
solution. For example

    try(Scope scope = new Scope())
    {
           Files.walk(scope, dir).flatMap(file->Files.lines(scope, file))
    }

    static Stream<Path> File.walk(scope, dir)
    {
        ... scope.onExit( free resources );
    }

or use thread local to further simply the syntax

    Scope.exec(()->
    {
           Files.walk(dir).flatMap(Files::lines);
   });

    static Stream<Path> File.walk(dir)
    {
        scope = Scope.current();
        if(scope==null) throw new Exception();
        return walk(scope, dir);
    }






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