@CleanUp better than JDK 7 try with resources?

[Fabrizio Giudici]

This blog post points out a limitation of try-with-resources that I
believe also affects @CleanUp:

http://java.dzone.com/articles/common-try-resources-idiom?utm_source=feedburner&utm_medium=feed&utm_campaign=Feed%3A+javalobby%2Ffrontpage+%28Javalobby+%2F+Java+Zone%29


Then it advices about better code, which unfortunately in
try-with-resources causes a double call to close. The same better code is
IMHO required by Lombok, which in this case doesn't cause the double call
to close. Summing up, @CleanUp seems still better than JDK 7.


--
Fabrizio Giudici - Java Architect, Project Manager
Tidalwave s.a.s. - "We make Java work. Everywhere."
fabrizio...@tidalwave.it
http://tidalwave.it - http://fabriziogiudici.it

[Tim]

I don't see how Lombok does better.

I think this is the code in question:

JDK7:

try (FileReader fr = new FileReader(file);

        BufferedReader br = new BufferedReader(fr)) {

    System.out.println(br.readLine());

}

Lombok:

@Cleanup FileReader fr = new FileReader(file);

@Cleanup BufferedReader br = new BufferedReader(fr);

System.out.println(br.readLine());

In both cases if there are no exceptions, there is a call to br.close() ( triggering a call to fr.close()) followed by a call to fr.close().  So fr.close() is called twice.  Isn't that right?

This seems like a pretty trivial concern.  It only matters if the wrapped class (FileReader in this example) doesn't tolerate having its close() called twice and the wrapping class (BufferedReader) has a close that calls the close of the wrapped class.  I think it would be hard to construct a sensible example of this.

-- Tim

[Reinier Zwitserloot]

The shown code (wrapping a call to FileInputStream inside a call to some other filtering inputstream or reader) is BROKEN.

In fact, the filtering stream does NOT need a @Cleanup guard at all, there's no need to close it. Sometimes this is useful in order to ensure content is 'flushed' out of the buffer, but usually it doesn't matter in exceptional conditions anyway, so long as someone attempts to close the thing that actually represents a system resource and could leak if we don't close it (i.e. the FileInputStream).

As Tim said, the correct approach is to have 2 @Cleanup-annotated variable declarations.

[Fabrizio Giudici]

Well, first I've lost a bit the context of this thread (my fault) because
in the meantime a lot of things happened to me. Trying to recover it...

On Mon, 21 May 2012 23:47:46 +0200, Reinier Zwitserloot
<rein...@gmail.com> wrote:

> The shown code (wrapping a call to FileInputStream inside a call to some
> other filtering inputstream or reader) is BROKEN.
>
> In fact, the filtering stream does NOT need a @Cleanup guard at all,
> there's no need to close it. Sometimes this is useful in order to ensure
> content is 'flushed' out of the buffer, but usually it doesn't matter in
> exceptional conditions anyway, so long as someone attempts to close the
> thing that actually represents a system resource and could leak if we
> don't
> close it (i.e. the FileInputStream).

I don't understand completely your sentence. Can you write a simple code
example?

[Reinier Zwitserloot]

FileInputStream represents a 'real' resource. If you don't close it, you leak real memory, and that's a real problem.

InputStreamReader does not represent a 'real' resource. If you don't close it but you DO close the thing it was wrapping, you do not leak real memory, and it's no big deal.

If you have this code:

@Cleanup InputStream is = new BufferedInputStream(new FileInputStream("/some/path/to/a/file"));

then you've written broken code (whether you use @Cleanup or ARM, doesn't matter) - you are now relying on the BufferedInputStream, i.e. the filtering stream, to forward close calls to the inner 'real' resource. However, maybe that goes wrong, or, maybe the creation of the filtering stream goes wrong. In either case, you're failing to close a 'real' resource, and thus you've created a real problem.

Therefore, the above code is NEVER acceptable, and relying on a filtering stream to forward a close call is pointless. Whatever you do, if you call new FileInputStream, then you need to stick 'fis.close();' somewhere in some guard block (or use @Cleanup or ARM on this FileInputStream), and if you want to read this thing via a filtering stream like BufferedInputStream, tough cookies, that does not absolve you of the need to @Cleanup/ARM the real resource.

But here's the trick: While you need to ARM/@Cleanup the 'real' resource, you DO NOT need to ARM/@Cleanup the filter, because that does not actually represent a resource. The following code:

byte[] b = new byte[4096];

{

@Cleanup FileInputStream rawIn = new FileInputStream("/path/to/some/file");

BufferedInputStream in = new BufferedInputStream(rawIn);

in.read(b);

in.close();

}

is NOT BUGGY - even though 'in' is not protected, it doesn't matter, because the relevant resource (rawIn) _IS_ protected. Assuming no exceptional exit occurs in this method, then in.close() effectively serves as a way to 'flush' the bufferedinputstream (irrelevant for reading, but very relevant for writing!) and if you forget to close/flush it your code is still buggy. If an exceptional exit does occur (reading the file causes an exception), then the 'in.close()' call is skipped and thus we fail-to-flush, but this is generally not important anyway (after all the I/O stream just failed on us, it's not gonna work anyway... or something else failed and whatever we wrote to the file is now irrelevant).

I usually program like this - I always throw .close() calls into the code in addition to the @Cleanup annotation, and I only @Cleanup resources that are 'real' and not virtual.

[Fabrizio Giudici]

On Mon, 04 Jun 2012 16:22:15 +0200, Reinier Zwitserloot
<rein...@gmail.com> wrote:

> I usually program like this - I always throw .close() calls into the code
> in addition to the @Cleanup annotation, and I only @Cleanup resources
> that
> are 'real' and not virtual.

Point taken. Actually - apart a few exceptions - I've checked that I've
mostly written correct code. But it was something "subliminal" as it
wasn't backed up by a conscious rationale (e.g. I've always put an
explicit close() more because of a matter of readability than
correctness). I guess a lot of people aren't getting the point, though.