Community
Participate
Working Groups
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. - in foo3 "throwException();" is a potential method exit point. We cannot be sure in this case, as a method may or may not throw an exception. I am not sure what is the right behavior in this case. Showing a warning at every potential method exit point will probably result in too many warnings, hence we can probably leave this as is. ------------------------------------------------------------------------------ void foo1(boolean a, boolean b, boolean c) throws Exception { FileReader reader = new FileReader("file"); if(a) return; //warning else if (b) reader.close(); else if(c) return; //warning reader.close(); } void foo2(boolean a, boolean b, boolean c) throws Exception { FileReader reader = new FileReader("file"); //warning if(a) throw new Exception(); else if (b) reader.close(); else if(c) throw new Exception(); reader.close(); } void foo3(boolean a, boolean b, boolean c) throws Exception { FileReader reader = new FileReader("file"); //warning if(a) throwException(); else if (b) reader.close(); else if(c) throwException(); reader.close(); } void throwException() throws Exception { throw new Exception(); } -------------------------------------------------------------------------------
(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!
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.
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.
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)
Released as commit e18b8152863f75de59815219afe0ab3ec946a798 for 3.8 M3.
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)
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.
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.
After more testing I released the additional fix as commit 9d00e6aaf0f2f4598b676da2bb31ba68b6d40cc8 for 3.8 M3
(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!
Verified for 3.8 M3 using build id: N20111022-2000.
