Bug 359334

Comment 1 Ayushman Jain 2011-09-29 03:03:20 EDT

(In reply to comment #0)
> Sigh... I realized this morning that analysis for resource leak warnings (bug #
> 349326) does not consider exceptions as method exit points
> 
> Consider the following three code snippets.
> 
> - foo1 and foo2 are essentially same, but result in different warnings. A
> statement like "throw new Exception();" is a method exit point, unless of
> course the exception is caught. Warnings in foo1 and foo2 should be consistent.

Agreed that in this case, we can treat the throwing of exception at par with a return and issue a "resource may not not be closed at this point" warning. Stephan, can you take a look when you get time? Thanks!

Comment 2 Stephan Herrmann 2011-09-29 08:32:32 EDT

I agree with both your comments:

(a) explicit throw should be handled just like return.

(b) leave it at ignoring potential exception exits 
    (see bug 349326 comment 58)

I'll post a patch for (a) - should be real easy to implement.

Comment 3 Stephan Herrmann 2011-09-29 18:58:54 EDT

Created attachment 204331 [details]
test & fix

Implementing this was actually trivial,
only EGit has some hiccups again, please ignore the duplication
of Olivier's commit regarding MethodVerifyTest.

Comment 4 Stephan Herrmann 2011-09-29 19:53:26 EDT

Created attachment 204333 [details]
patch with cleanup

I performed two-level cleanup:
- no more EGit glitches in this patch
- refactoring: removed method parameters that I introduced in early
  versions of bug 349326
  (unused since bug 349326 comment 58)

Comment 5 Stephan Herrmann 2011-09-29 19:57:30 EDT

Released as commit e18b8152863f75de59815219afe0ab3ec946a798 for 3.8 M3.

Comment 6 Deepak Azad 2011-09-30 01:20:09 EDT

The fix is not yet perfect :)

There is an incorrect warning in foo2(), the throw statement inside the try block is not even a method exit point. However, things work fine in foo1 and foo3.
------------------------------------------------------------------------------
	void foo1() throws Exception {
		FileReader reader = new FileReader("file"); // warning

		try {
			reader.read();
			return;
		} catch (Exception e) {
			throw new Exception();
		} finally {
			reader.close();
		}
	}

	void foo2() throws Exception {
		FileReader reader = new FileReader("file"); // warning

		try {
			reader.read();
			throw new Exception(); // Incorrect warning
		} catch (Exception e) {
			throw new Exception();
		} finally {
			reader.close();
		}
	}

	void foo3() throws Exception {
		FileReader reader = new FileReader("file"); // warning

		try {
			reader.read();
			throw new Exception();
		} finally {
			reader.close();
		}
	}
------------------------------------------------------------------------------

(Maybe you already know this - Mark occurrences can help you in quickly finding method exit points, see http://thecoderlounge.blogspot.com/2011/06/jdt-tip-mark-occurrences-in-java-editor.html)

Comment 7 Stephan Herrmann 2011-10-01 13:41:38 EDT

Created attachment 204420 [details]
additional fix

(In reply to comment #6)

You are quite a test case generator, wow :)

Is this
  try {
    throw new Exception();
    // some here
  } catch (Exception ex) {
    // stuff here
  }
the Java lingo for goto? :)

Without some tricks null-analysis is actually not a champ for handling
method exit points (maybe def-init would be a better fit here, but I don't
plan to do a practical comparison).

Anyway, I found a place for storing the flowInfo of the finally block
so that during error reporting I can re-check if that info has definitely
seen close(). This has to be checked for all levels of scopes between the
exit point and the scope that defined the resource variable.

New tests also include different nested structure inside the finally block
where we can/cannot definitely see a close(). One test was inspired by
org.eclipse.jdt.core.tests.util.Util, where half of the patch still 
produced a spurious warning, which is now gone, too. We now profit
from the fact that the analysis indeed recognizes that a finally block
like this cannot leak the resource:
            if (reader != null)
                 try {
                     reader.close();
                 } catch (java.io.IOException io) {}
Although we have a reachable else branch without a close() it cannot leak
reader because here reader == null :)

Also, I removed a leak in my own implementation :)
We no longer hold on to any FakedTrackingVariable after they're used, 
there're too many references in there including a scope.

Comment 8 Stephan Herrmann 2011-10-01 14:30:28 EDT

Created attachment 204421 [details]
additional fix improved

I could still see one unwanted warning related to throw exits in
org.eclipse.jdt.internal.core.ClassFile.getBytes()

Here the close in the finally is indirect by calling closeZipFile(zip).
At a closer look the bogus warnign was caused by a logic bug in the new
method mergeCloseStatus. Fixed.

Comment 9 Stephan Herrmann 2011-10-01 17:54:51 EDT

After more testing I released the additional fix as
commit 9d00e6aaf0f2f4598b676da2bb31ba68b6d40cc8 for 3.8 M3

Comment 10 Deepak Azad 2011-10-03 02:10:14 EDT

(In reply to comment #7)
> You are quite a test case generator, wow :)
I just look at the warnings in jdt.core :)

The fix looks good. Thanks again for the quick response!

Comment 11 Srikanth Sankaran 2011-10-25 06:25:15 EDT

Verified for 3.8 M3 using build id: N20111022-2000.