Community
Participate
Working Groups
When compiled using 3.8M3 the test suite org.eclipse.jdt.compiler.tool.tests raises 8 warnings like "Resource leak: 'fileManager' is never closed". It seems that all these warnings are justified and should be fixed in the code.
Created attachment 206678 [details] proposed patch This patch resolves the warnings by adding appropriate close() calls. When I run the tests in the IDE I get one failure, though: org.eclipse.jdt.compiler.tool.tests.CompilerToolTests.testFileManager2(): Expected: "X2.javaX.java" but got "X.javaX2.java". A simple ordering issue, not sure if this is a known issue and this is independent of my patch anyway.
(In reply to comment #1) Thats strange. Adding close calls seems to have changed the ordering of CU's in the fileManager. May be worth investigating why this happened, just to be sure there's no auxilliary problem that arose because of the new close() call.
(In reply to comment #2) > (In reply to comment #1) > Thats strange. Adding close calls seems to have changed the ordering of CU's in > the fileManager. May be worth investigating why this happened, just to be sure > there's no auxilliary problem that arose because of the new close() call. I tried to mention that I see this failure also without the added close calls. BTW: the doc of JavaFileManager.list(..) doesn't seem to make any guarantees regarding the order in the result.
(In reply to comment #3) > I tried to mention that I see this failure also without the added close calls. Ah! I see. I applied your patch and ran the test. Passes on my machine (Win32) everytime.Maybe on linux it gives an incorrect ordering sometimes. So, unless you see the incorrect order everytime you run the test, I guess the patch should be good.
(In reply to comment #4) > (In reply to comment #3) > > I tried to mention that I see this failure also without the added close calls. > Ah! I see. I applied your patch and ran the test. Passes on my machine (Win32) > everytime.Maybe on linux it gives an incorrect ordering sometimes. So, unless > you see the incorrect order everytime you run the test, I guess the patch > should be good. I do see it everytime. Regardless of with/without the close() calls, regardless of the JRE used: I never see this one test pass. Perhaps I should just release this patch and open a separate bug on the ordering issue? OTOH, I see no reason to actually check the ordering so we could simply make the test order-independent, right?
(In reply to comment #5) > OTOH, I see no reason to actually check the ordering so we could simply > make the test order-independent, right? Exactly. I meant to write the same in my previous comment :)
Created attachment 206697 [details] patch with sorting (In reply to comment #6) > (In reply to comment #5) > > OTOH, I see no reason to actually check the ordering so we could simply > > make the test order-independent, right? > > Exactly. I meant to write the same in my previous comment :) Here's a variant with sorting the result before comparison. I've tried it with a number of different JREs. To be released unless anybody shouts :)
Released for 3.8 M4 via commit 2a6ccc4326c5f51715033b769f3fad2752538c59
The change in AbstractCompilerToolTest.java doesn't look completely correct. It is ok to close the manager if it is coming from the standard manager, however it should not be closed if it is being passed as the argument.
(In reply to comment #9) > The change in AbstractCompilerToolTest.java doesn't look completely correct. It > is ok to close the manager if it is coming from the standard manager, however > it should not be closed if it is being passed as the argument. Are you saying this, because you think it could break the tests? -> currently doesn't. Do you see danger that future tests will break because of this? Or is it more the conceptual question, why the warning was issued in the first place (since the current method doesn't have the sole responsibility)?
(In reply to comment #10) > Are you saying this, because you think it could break the tests? > -> currently doesn't. Do you see danger that future tests will > break because of this? Yes, the current tests don't fail because of this, but it could break future tests and I don't think this is a good idea to close the file that is passed as an argument. > Or is it more the conceptual question, why the warning was issued in the > first place (since the current method doesn't have the sole responsibility)? Yes, it is also conceptual. I think you wan to address this through bug 361407.
Filed bug 365710 to take care of the changes.
Verified for 3.8M4 using build I20111202-0800
