Community
Participate
Working Groups
Created attachment 208810 [details] Test sources close() method is not executed, probably when calling method ends with return statement with condition. (Produces bytecode is somewhat messy and do not handle all execution paths) See attached sources for test case. Code compiled with Oracle javac runs normally. Tested on Eclipse 3.8M4 and 3.7.1 JDK is 1.7u2 When compiled with Eclipse, output is something like: Test.Test() Resource.Resource() @150bd4d Test.close() ERROR: some resources was not closed -> test.Resource@150bd4d When compiled with javac, output is: Test.Test() Resource.Resource() @12b6651 Resource.close() @12b6651 Test.close()
Created attachment 208816 [details] test & preliminary fix Indeed! The generated code contains a premature ireturn that bypasses the implicit call to close(). The patch contains a simpler test case and a preliminary fix. Inside ReturnStatement.analyseCode(..) we didn't recognize that we are in a context that indeed has a subroutine to be called before returning, and thus the flag IsReturnedValue was wrongly set. The flag then cause generating the bogus ireturn. The patch fixes that by searching also for an enclosing try statement with resources (which doesn't have an InsideSubRoutineFlowContext but still needs to call a subroutine).
(In reply to comment #1) > Created attachment 208816 [details] > test & preliminary fix Stephan, thank you! With patch it seems all work good. I compiled patched org.eclipse.jdt.core bundle for myself and put it into Eclipse plugins folder.
(In reply to comment #2) > (In reply to comment #1) > > Created attachment 208816 [details] > > test & preliminary fix > > Stephan, thank you! > With patch it seems all work good. Thanks for confirming! The patch still needs to be reviewed, but we certainly want this fixed no later than M5. @Ayush: do you think we should actually have an InsideSubRoutineFlowContext in this situation, or do you consider the location where my patch applies safe?
Stephan, we have a test org.eclipse.jdt.core.tests.compiler.regression.TryWithResourcesStatementTest.test030() that tests for this and asserts that the resource closes do happen. Have you been able to find out what is special about the current test case that causes this problem ?
(In reply to comment #4) > Stephan, we have a test > org.eclipse.jdt.core.tests.compiler.regression.TryWithResourcesStatementTest.test030() > that tests for this and asserts that the resource closes do happen. > > Have you been able to find out what is special about the current test case that > causes this problem ? The difference is: - the return statement returns a value provided by a binary expression of type boolean The bit that was missing from my explanation in comment 1 is: Inside BinaryExpression.generateCode(..) we check for ASTNode.IsReturnedValue, iff set we generate an immediate ireturn for the true-case. If IsReturnedValue is set in a t-w-r without a finally block we miss to generate the close() call for the true-case. The test030 you mention doesn't involve a return value.
Created attachment 208929 [details] Alternate patch I didn't find anything particularly wrong with the patch already proposed, but I like this alternate patch (slightly) better. This is still under test. We now recognize the presence of autocloseables in the picture as an additional case where we cannot abruptly return while computing the return value. (as is already done for synchronized block.) The original patch tends to pass around null values and has to guard against one case where this passing around would result in an NPE. So I feel the current patch is somewhat cleaner. Stephan, let me know if I have overlooked something.
(In reply to comment #6) > Created attachment 208929 [details] > Alternate patch This patch also includes an additional test.
(In reply to comment #6) > We now recognize the presence of autocloseables in the > picture as an additional case where we cannot abruptly > return while computing the return value. (as is already > done for synchronized block.) The main difference I see between both patches is whether or not saveValueNeeded is set, the impact of which I don't immediately understand. Analogy to a regular finally recommends yes, analogy to synchronized says no. Hm... > Stephan, let me know if I have overlooked something. At a quick glance your patch looks good to me. My patch was more for checking my theory about the root cause and I wasn't quite sure about all its side effects. Please feel free to take the bug if you want to release your patch.
(In reply to comment #8) > The main difference I see between both patches is whether or not > saveValueNeeded is set, the impact of which I don't immediately understand. There is not much really. This boolean short circuits setting of the ASTNode.IsReturnedValue bit, which we are achieving otherwise anyways. Released fix and tests via http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=667b6c6169416d9b79d86e73c3fa9c45930c36a8 for 3.8 M5. Ayush, could you release these for 3.7.2 and 3.6.2+Java7 streams ? TIA.
Dani, we need to back port this fix too. In some not very uncommon cases, we will fail to close the resources specified in a TWR. Fix is clean IMO.
+1 for backport.
Released in 3.7 maintenance via commit 4bd87a9bbcc7e440b98ccfd3be7628d44517951c
Released in 3.6.2+Java7 branch via http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=117d4b22e0330a291663c9fe32b87118e24c700b
(In reply to comment #13) > Released in 3.6.2+Java7 branch via > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=117d4b22e0330a291663c9fe32b87118e24c700b Please not that you still need to do the build input manually (as in contrast to 3.7.2), see bug 364676.
Verified for 3.7.2 RC2 with build M20120118-0800
Verified for 3.8M5 using I20120122-2000
Should this still be verified for 3.6.2+J7 (see target) or is it OK to mark VERIFIED?
(In reply to comment #17) > Should this still be verified for 3.6.2+J7 (see target) or is it OK to mark > VERIFIED? Verified in R3_6_maintenance_Java7.
