Community
Participate
Working Groups
Created attachment 209453 [details] resource leak problems When compiling the Eclipse SDK with the latest patch in bug 358903 I still see 22 warnings about definite resource leaks. Some of these are false positives which should be avoided. Some examples of trouble caused by specific syntax: In org.eclipse.equinox.internal.util.io.ExternalizableDictionary: ObjectInputStream in = loader == null ? new ObjectInputStream(is) : (ObjectInputStream) new XObjectInputStream(loader, is); -> it seems we choke on the conditional expression? In org.eclipse.equinox.internal.p2.artifact.repository.RawMirrorRequest: destination = handler.link(new ProcessingStep[] {new MD5Verifier(descriptor.getProperty(IArtifactDescriptor.DOWNLOAD_MD5))}, destination, monitor); -> how to handle resources in an array initializer? Some others are related to special data flows not being understood. For the records I'm attaching the list of problems I currently see.
Bug 358903 introduced a disabled test that should be investigated in the context of this bug, too: ResourceLeakTests#_test061q
Created attachment 209555 [details] resource leak warnings (In reply to comment #0) > Created attachment 209453 [details] > resource leak problems > > When compiling the Eclipse SDK with the latest patch in bug 358903 > I still see 22 warnings about definite resource leaks. > Some of these are false positives which should be avoided. I see 57 warnings, see attachment. My setup is - Fresh workspace. - Import all plugin projects with source whose name start with org.eclipse.
I see one more here: - Resource leak: 'out' is not closed at this location EclipseTestRunner.java /org.eclipse.test/src/org/eclipse/test line 692 Java Problem It uses this pattern: void foo(File outfile) { OutputStream out= System.out; if (outfile != null) { try { out = new FileOutputStream(outfile); } catch (java.io.IOException e) { throw new RuntimeException(e); } } setOutput(out); } private void setOutput(OutputStream out) { }
(In reply to comment #2) > > When compiling the Eclipse SDK with the latest patch in bug 358903 > > I still see 22 warnings about definite resource leaks. > > Some of these are false positives which should be avoided. > > I see 57 warnings, see attachment. > > My setup is > - Fresh workspace. > - Import all plugin projects with source whose name start with org.eclipse. Some differences (using the released version of bug 358903 plus the pending patch for bug 368709): I don't see the warnings in org.eclipse.pde.internal.ui.launcher.OpenLogDialog org.eclipse.team.internal.ccvs.core.CVSTeamProvider org.eclipse.update.internal.jarprocessor.ZipProcessor org.eclipse.ltk.internal.core.refactoring.history.RefactoringHistoryManager etc. All these locations look OK to me. Which version were you using? In my test workspace I currently see 25 resource leak warnings in org.eclipse.* (not counting jgit).
I am compiling 3.8 M4 source against the latest in JDT/Core master which includes the fix for bug 368709.
(In reply to comment #4) > org.eclipse.pde.internal.ui.launcher.OpenLogDialog private void readFile(PrintWriter writer) throws FileNotFoundException, IOException { BufferedReader bReader = new BufferedReader(new FileReader(logFile)); // warning while (bReader.ready()) writer.println(bReader.readLine()); } The warning I see is : "Resource leak: 'bReader' is never closed". This warning looks correct to me. But I have no clue why you do not see it.
(In reply to comment #6) > (In reply to comment #4) > > org.eclipse.pde.internal.ui.launcher.OpenLogDialog > > private void readFile(PrintWriter writer) throws FileNotFoundException, > IOException { > BufferedReader bReader = new BufferedReader(new FileReader(logFile)); // > warning > while (bReader.ready()) > writer.println(bReader.readLine()); > } > > The warning I see is : "Resource leak: 'bReader' is never closed". This warning > looks correct to me. But I have no clue why you do not see it. Here's a clue: install a 1.4 JRE and you'll see that pde.ui (and all projects configured for 1.4) don't participate in this game: for the compiler they have no notion of resources because they have no type Closeable (or even AutoCloseable). Here's my current steps and statistics: From my development Eclipse (SDK + Mylyn + EGit) install all org.eclipse.* plug-ins as projects with source folder - close *mylyn* and *egit* Should be roughly 224 projects now. Then fix broken Eclipse SDK sources (is that the code being shipped????): - Fix 3 type errors in org.eclipse.equinox.util (ternary expression with type variables) - Change all org.eclipse.jetty.* to Java2SE-6 (they use @Override for implementors) - Remove one bogus package export from org.eclipse.pde.api.tools To make resource leaks stick out I set workspace preference to report them as errors. Using build I20111209-1447 I see: 82 resource leaks Using today's HEAD (runtime workbench) I see: 31 resource leaks As a next step I will classify these 31 problems.
(In reply to comment #7) > Here's a clue: install a 1.4 JRE and you'll see that pde.ui (and all projects > configured for 1.4) don't participate in this game: for the compiler they have > no notion of resources because they have no type Closeable (or even > AutoCloseable). Ha! :-) Now I added the 1.4 JRE to the workspace, but 1.4 projects are still using the 1.5 JRE. I guess something went wrong in the workspace, I will try to fix the problem and report the numbers here.
(In reply to comment #8) > Now I added the 1.4 JRE to the workspace, but 1.4 projects are still using the > 1.5 JRE. I guess something went wrong in the workspace, I will try to fix the > problem and report the numbers here. This is because of bug 369480 :(
We report a potential leak warning here, but I don't see how a potential leak can come at the throw statement class MyException extends Exception{} class X{ void foo(String fileName) throws IOException, MyException { FileReader fileRead = new FileReader(fileName); BufferedReader bufRead = new BufferedReader(fileRead); LineNumberReader lineReader = new LineNumberReader(bufRead); try { while (lineReader.readLine() != null) { bufRead.close(); callSome(); // only this can throw MyException } } catch (MyException e) { throw e; // Pot. leak reported here } bufRead.close(); } private void callSome() throws MyException { } }
Another example with unchecked exceptions. The analysis seems to think that the resources is always closed and gives a "can be managed with twr" warning void countFileLines(String fileName) throws IOException { FileReader fileRead = new FileReader(fileName); BufferedReader bufRead = new BufferedReader(fileRead); LineNumberReader lineReader = new LineNumberReader(bufRead); while (lineReader.readLine() != null) { if (lineReader.markSupported()) throw new IOException(); bufRead.close(); } bufRead.close(); } The throw new IOException() exposes an exit path where resource is not closed.
public void goo() throws IOException { // LineNumberReader o = new LineNumberReader(null); LineNumberReader o = null; try { o = new LineNumberReader(null); } catch (NumberFormatException e) { } } In this case we get a pot. resource leak, but o is never really closed. If you uncomment the first statement, we get a resource leak. Seems weird.
The following warning looks wrong Resource leak: 'stream' is not closed at this location ProcessingStepHandler.java /org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/provisional/p2/artifact/repository/processing line 188 private ArtifactOutputStream getArtifactStream(OutputStream stream) { OutputStream current = stream; while (current instanceof ProcessingStep) current = ((ProcessingStep) current).getDestination(); //warning if (current instanceof ArtifactOutputStream) return (ArtifactOutputStream) current; return null; }
(In reply to comment #13) > The following warning looks wrong > > Resource leak: 'stream' is not closed at this location > ProcessingStepHandler.java > /org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/provisional/p2/artifact/repository/processing > line 188 > > private ArtifactOutputStream getArtifactStream(OutputStream stream) { > OutputStream current = stream; > while (current instanceof ProcessingStep) > current = ((ProcessingStep) current).getDestination(); //warning > if (current instanceof ArtifactOutputStream) > return (ArtifactOutputStream) current; > return null; > } Why wrong? Before the assignment 'current' holds a resource. This binding will be lost by the assignment so we don't know what will happen with stream. The only fishy thing I can see is that it should better be a pot.leak warning, because we didn't create the stream within this method. Is that what you meant?
(In reply to comment #14) > Why wrong? > Before the assignment 'current' holds a resource. This binding will > be lost by the assignment so we don't know what will happen with stream. On the given line 'current' is reassigned, but the warning complains about 'stream'. > The only fishy thing I can see is that it should better be a pot.leak > warning, because we didn't create the stream within this method. > Is that what you meant? As per bug 362332 comment 5 we should not be reporting anything for 'stream' here. Or should we?
I can see the following code pattern a bunch of times in the SDK. This example is from JDT UI. I think the code is perfectly fine here, but yet we have a warning. - If an exception is thrown on creation of a resource, does that resource has to be closed? I think not. - If an exception is thrown by 'close()' itself, can you do anything about closing the resource? These are the 2 code paths I see that probably result in the warning. Resource leak: 'stream' is not closed at this location ProfileStore.java /org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/preferences/formatter line 270 ------------------------------------------------------------------------------- public void writeProfilesToFile(Collection<Profile> profiles, File file, String encoding) throws CoreException { final OutputStream stream; try { stream= new FileOutputStream(file); try { writeProfilesToStream(profiles, stream, encoding, fProfileVersioner); } finally { try { stream.close(); } catch (IOException e) { /* ignore */ } } } catch (IOException e) { throw createException(e, FormatterMessages.CodingStyleConfigurationBlock_error_serializing_xml_message); //warning on this line } } -------------------------------------------------------------------------------
I'll try to find some root causes in this field. If there's a lot of different issues involved I will split out individual bugs that will have to be deferred post Juno then.
(In reply to comment #10) > We report a potential leak warning here, but I don't see how a potential leak > can come at the throw statement > class MyException extends Exception{} > class X{ > void foo(String fileName) throws IOException, MyException { > FileReader fileRead = new FileReader(fileName); > BufferedReader bufRead = new BufferedReader(fileRead); > LineNumberReader lineReader = new LineNumberReader(bufRead); > try { > while (lineReader.readLine() != null) { > bufRead.close(); > callSome(); // only this can throw MyException > } > } catch (MyException e) { > throw e; // Pot. leak reported here > } > bufRead.close(); > } > private void callSome() throws MyException > { > > } > } I filed bug 370424 for the underlying problem in our analysis machinery. The example in that bug is the same as here with simple substitutions: - resource allocation: assign null - resource close: assign nonnull - force diagnostic by 'throw' : force diagnostic by dereference
Created attachment 210478 [details] Tests & fix This patch fixes all plain false positives I could find in the entire Eclipse SDK, including the examples from comment 3, comment 11, comment 13 and comment 16 and some more. Most items were straight-forward application of existing concepts into hidden corners of the implementation, including: - check the effect of a resource allocation only *after* analysing possible exceptions raised by the allocation - consider all resources that are wrapped in an array as passed-to-outside since from there on we lose track of their state. - propagate flags OWNED_BY_OUTSIDE and SHARED_WITH_OUTSIDE along a few more paths - better detection of field references (checking (bits & Binding.FIELD) is *not* sufficient for QualifiedNameReference!) - don't complain against variables on branches where they are definitely unassigned - respect if a resource was closed in the finally block of any enclosing try statement - better detect potential problems, in some cases they are signaled by pot.nonnull instead of the expected pot.null. The only new concept is that we now include resource analysis into deferred checking for loops, which required a little refactoring in LoopingFlowContext. I am yet to find an example where this would require corresponding changes in FinallyFlowContext. Example from comment 10 is postponed due to bug 370424. Example from comment 12 suffers from the way info from try and from catch are merged, after the try-catch we no longer have definite info - I currently don't see an easy solution.
Created attachment 210479 [details] Remaining leak problems With the patch from comment 19 compiling the entire Eclipse SDK is now down at reporting EIGHT definite resource leaks. I'm attaching the list that I see (sources are a snapshot from a few weeks ago). According to bug 358903 comment 13 the plan was to reduce almost 100 warnings against the SDK by estimated 30%. I think reducing the warnings by more than 90% should suffice for that plan :)
After triple checking I released commit b5977e771dcaa856815c805c4cdc6cf2a2c18650 for 3.8 M6.
*** Bug 370702 has been marked as a duplicate of this bug. ***
Verified for 3.8 M6 using Build id: I20120306-0800
