Revisiting MayHoldCloseableResource

[Joe Bowbeer]

Another unfortunate thing is the name close() which is, I think, a little
stricter than we want here.  dispose() or release() would have been better
-- because they convey an intent not to use anymore, but do not imply that
a synchronous transition to the "closed" state should be visible to every
observer immediately.

But, alas, neither dispose() nor release() are viable alternatives at this
time.


On Tue, Aug 13, 2013 at 10:24 AM, 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 closed. ByteArrayInputStream etc. already show that we can't
> effectively 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.
>
>
>
>>
>>      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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/lambda-libs-spec-experts/attachments/20130813/318e2a2d/attachment.html