Community
Participate
Working Groups
4.2 M3. Consider the following code: public void handle(ServletResponse response) throws IOException { PrintWriter writer = response.getWriter();//resource leak reported here writer.write("Hello"); writeMore(response); } private void writeMore(ServletResponse response) throws IOException { response.getWriter().write(" World"); } JDT is reporting a resource leak error here, but in fact it is not because the writer was not created in that scope and it is not my responsibility to close it. If you obtained a closeable from a method call, the method contract might state that it is not the caller's responsibility to close it. This is common in servlet programming where the servlet container is responsible for closing any streams after invoking the servlet. I would be ok with this being reported as a potential leak, rather than a definite leak.
(In reply to comment #0) > I would be ok with this being reported as a potential leak, rather than a > definite leak. We can do that.
Postponing to M5 in favor of the end-game in bug 186342. During M5 I will address this one jointly with some related bugs: bug 358903, bug 361073, bug 360908 and bug 361407.
See bug 365710 for a related case: resource is obtained by reading from a field relative to a method argument (which is kind-of the inverse of bug 361407). Common theme: the rhs in the resource assignment needs further checking to determine whether the current method has the sole responsibility for closing the resource.
Fix is contained in the patch in bug 358903 comment 20. See the test063x group of tests. Note that resources obtained by reading a field are now treated in the same way as those obtained from a method call. Additionally, assigning a resource to a field is now treated similar to passing it to another method. However, in the method case we still raise a potential problem, whereas a resource assigned to a field is likely intended to outlive the current method execution, so no warning is issued for those.
Here's what the latest fix in bug 358903 does exactly: We classify resources using the following flags: // the resource is shared with outside code either by // - passing as an arg in a method call or // - obtaining this from a method call or array reference // Interpret that we may or may not be responsible for closing private static final int SHARED_WITH_OUTSIDE = 2; // the resource is likely owned by outside code (owner has responsibility to close): // - obtained as argument of the current method, or via a field read // - stored into a field // - returned as the result of this method private static final int OWNED_BY_OUTSIDE = 4; For the first group we never signal "Resource leak", only "Potential ...". For the second group we never signal anything. Only for resources that fall in neither of these groups do we assume that the current method has the full responsibility and hence a definite "Resource leak" can be signaled with confidence. The example in comment 0 falls into the group SHARED_WITH_OUTSIDE, and will only raise potential leak warnings.
Resolved by commit 8d45cb26fc5ad244f93e8632d761d46ad4a120cf on behalf of bug 358903.
I think this fix does not understand complex expressions inside the messageSend. Eg class Snippet{ public void handle() throws IOException { PrintWriter writer = new PrintWriter(new File("")); // Resource leak writer.write("Hello"); new Snippet(writer.append('w')); } public Snippet (PrintWriter pw) { } } This still gives a resource leak. Should I mark this as verified and open a new bug?
(In reply to comment #7) > I think this fix does not understand complex expressions inside the > messageSend. Eg Similar example new Snippet(new PrintWriter(new FileOutputStream(new File("")), true).append('c'));
(In reply to comment #7) > I think this fix does not understand complex expressions inside the > messageSend. Eg > > class Snippet{ > public void handle() throws IOException { > PrintWriter writer = new PrintWriter(new File("")); // Resource leak > writer.write("Hello"); > new Snippet(writer.append('w')); > } > public Snippet (PrintWriter pw) { > > } > } > > This still gives a resource leak. > Should I mark this as verified and open a new bug? What exactly do yo expect? Should the analysis "know" that the append method is an identity mapping? If you think that's worth a bug it might read: "add white lists of methods on subtypes of closeable that always return 'this' for consideration during resource leak analysis." However, I wouldn't support such an RFE because I think things are getting out of proportion here. As for this bug: the resource *is* created in the local scope and there is no close() call nor an explicit data flow involving the resource. Do you see any requirements of this bug not being met? Nobody forces the user to painfully *avoid* any local variables which we could analyze pretty well.
(In reply to comment #9) > What exactly do yo expect? Should the analysis "know" that the append method is > an identity mapping? Actually thats not what I was trying to imply though my examples seem to have been distracting. I thought when we encounter resource.someMethod(), we convert the warning to "pot. resource leak" because someMethod may be closing the resource after doing other things. But I see thats its not a very intelligent code pattern and we don't support that, so this behaviour looks consistent. Sorry for the false alarm Verified for 3.8M5 using build I20120122-2000
