Resorce Leak compiler error/warning (detection) has some problem

[mmariotti]

Resorce Leak compiler error/warning (detection) has some problem

[stephan-herrmann]

Testing on JDK-17 the compiler informs me

The resource type ExecutorService does not implement java.lang.AutoCloseable

Testing on JDK-21 plenty of warnings reported as missing are actually shown.

@mmariotti please let me know which JDK version you build against and your compliance settings.

[stephan-herrmann]

All test cases with a lambda like x -> Executors.newCachedThreadPool(x) are special:

Currently, the analysis fails to see that the expression Executors.newCachedThreadPool(x) is an abridged return statement. I'll provide a PR to fix this, but then the analysis can hardly know how the lambda will be used.

For any added precision we probably need an @Owning or @NotOwning annotation on the functional type of the lambda, in order to decide whether some piece of code will eventually sign as responsible for closing.

I doubt, though, that we have a realistic chance to trace the data flow through any methods of Optional or such.

[mmariotti]

@stephan-herrmann I changed the test class to be compatible with java 11 on, the result is the same using:

• Eclipse Adoptium jdk-21.0.2.13-hotspot
• Eclipse Adoptium jdk-17.0.7.7-hotspot
• Eclipse Adoptium jdk-11.0.18+10

I uploaded the sample PoC here: https://github.com/mmariotti/eclipse-jdt-2207

The project is complete with Eclipse Project specific settings files, if you import it you should see exactly what I see.

I enabled the "potential-resource-leak" detection too (in project => properties => java compiler => error/warnings => potential resource leak)

It seems to me that it potential-detector is reporting 100% correct effective-leaks, not potential-leaks.
Conversely, the effective-detector seems to report potential-leaks.
Maybe in this version of Eclipse the two detectors have been incorrectly swapped?

Here you are a screenshot for brevity (resource leaks: WARN - yellow | potential: INFO - teal):

Here is the summary table:

+----------------------+-------------+---------------+----------+----------+----------+
| type                 | notAssigned | localVariable | managed  | field    | returned |
+----------------------+-------------+---------------+----------+----------+----------+
| expected             | WARN        | WARN          | NO-WARN  | NO-WARN  | NO-WARN  |
+----------------------+-------------+---------------+----------+----------+----------+
| direct               | FalseNeg    | FalseNeg      |          |          |          |
| supplier_lambdaExpr  | (misplaced) | (misplaced)   | FalsePos | FalsePos | FalsePos |
| supplier_lambdaExpr2 | FalseNeg    | FalseNeg      |          |          |          |
| supplier_lambdaBlock | FalseNeg    | FalseNeg      |          |          |          |
| supplier_methodRef   | FalseNeg    | FalseNeg      |          |          |          |
| function_lambdaExpr  | (misplaced) | (misplaced)   | FalsePos |          | FalsePos |
| function_lambdaExpr2 | FalseNeg    | FalseNeg      |          |          |          |
| function_lambdaBlock | FalseNeg    | FalseNeg      |          |          |          |
| function_methodRef   | FalseNeg    | FalseNeg      |          |          |          |
+----------------------+-------------+---------------+----------+----------+----------+

All test cases with a lambda like x -> Executors.newCachedThreadPool(x) are special:
Currently, the analysis fails to see that the expression Executors.newCachedThreadPool(x) is an abridged return statement. I'll provide a PR to fix this, but then the analysis can hardly know how the lambda will be used.

Indeed. Please note the singular behavior of the effective-detector in method public void t_function_lambdaExpr_field(String str): it is, correctly, not signaling the warning (while its counterpart public void t_supplier_lambdaExpr_field(TestAutoCloseable t) is).

It should be sufficient to check if the lambda is returning the AutoCloseable (just like a Supplier or Function will do), if it is possible to do so, to suppress the warning, because you won't kwow if/how the caller would close it.

For any added precision we probably need an @owning or @NotOwning annotation on the functional type of the lambda, in order to decide whether some piece of code will eventually sign as responsible for closing.

I doubt, though, that we have a realistic chance to trace the data flow through any methods of Optional or such.

I agree, nevertheless I think the main requisite is to avoid false positives, at least to some extent. And obviously you can't follow the data flow, but it can be fully ignored:

Take this method:

public void t_supplier_lambdaExpr_localVariable(TestAutoCloseable t)
{
    /* CORRECT (but misplaced)
     * expected: WARNING
     * actual:   WARNING
     */
    TestAutoCloseable t2 = Optional.ofNullable(t).orElseGet(() -> TestAutoCloseable.newInstance());
    t2.toString();
}

Leave out Optional and lambdas, focus on t2: it is an AutoCloseable local variable assigned somehow.
We can just assume it has not been closed.

[stephan-herrmann]

@stephan-herrmann I changed the test class to be compatible with java 11 on, the result is the same using:

OK, as now you avoid the JDK classes that have changed between 17 and 21 the JDK version is no longer relevant. This should ensure we are seeing the same things :)

First observation: whenever a test case acquires the resource using TestAutoCloseable.newInstance() the analysis cannot know, whether:

• the current method is responsible for closing the resource, or
• the resource has been retrieved from some cache of resources for long time use, and some other part of the code will eventually close it (perhaps on application shut down).

That's why none of the problems reported are as definite resource leaks. This is by design.

I agree, nevertheless I think the main requisite is to avoid false positives ...

When the compiler falsely reports a definite resource leak, that's a bug (and in the lambda case I have a fix already).

When the analysis doesn't come to a definite conclusion (neither definite leak, nor definitely clean) then reporting a potential problem is the best we can do and it will be difficult to argue that the report is a false positive. There is no such bias as "false positives are worse than false negatives".

That said, I don't see much leeway in improving the "naked" resource analysis, but the way forward is to use @Owning / @NotOwning. If you haven't already seen I recommend you read https://eclipse.dev/eclipse/news/4.31/jdt.php#annotation-based-resource-analysis

Then we can use your test cases to discuss if they can be improved such that most of the potential issues can be analyzed with certainty. Will you join me in that task?

[stephan-herrmann]

FYI, test cases in #2257 already contain a few examples how annotations can help for some lambda situations.

Given that the functional interfaces for many such situations are library classes this entails that we want #2258 sooner rather than later :) - I'll try to schedule this for, say, 2024-09.

[stephan-herrmann]

#2257 fixed the lambda case. You may want to pickup the next I-build when it appears at https://download.eclipse.org/eclipse/downloads/index.html and re-test the issue. If you find any use case that looks wrong despite the explanations I gave above, feel free to re-open and argue from which information the compiler should conclude differently than it does.

[akurtakov]

Patch for this one seems to have caused some failures in I-build https://download.eclipse.org/eclipse/downloads/drops4/I20240331-1800/testresults/html/org.eclipse.jdt.core.tests.compiler_ep432I-unit-cen64-gtk3-java17_linux.gtk.x86_64_17.html

[stephan-herrmann]

Patch for this one seems to have caused some failures in I-build https://download.eclipse.org/eclipse/downloads/drops4/I20240331-1800/testresults/html/org.eclipse.jdt.core.tests.compiler_ep432I-unit-cen64-gtk3-java17_linux.gtk.x86_64_17.html

Thanks.
Obviously it doesn't make sense to run tests which use a lambda expression with compliance 1.7.
Reminder to self: new tests must be run locally against all compliance levels since jenkins PR jobs test less then what production builds do.
Sorry for the noise.

@mmariotti As this is only a problem of the test, not of the implemention, this build should be good for verifying the fix: https://download.eclipse.org/eclipse/downloads/drops4/I20240331-1800/

[mmariotti]

@stephan-herrmann I just downloaded I20240331-1800, but nothing has changed: false positives and negatives are exactly the same as of 2024-03.

Look at t2: the message highlighted is NOT a potential-resource-leak, it is effective!
Conversely, the message on lambda, that is a potential

---

Here, too: how can this be a potential-resource-leak? It is quite obvious it is an effective one and no lambda is involved.

---

Without going deeper with lambdas & co. detectors are a mess when just methods are involved:

---

I have limited time available, but I'll try to learn how detection works and see if I can propose some fix.

However, before focusing on lambdas and "owning" annots I think it must work, at least decently, with legacy code.

[stephan-herrmann]

I think it must work, at least decently, with legacy code.

I still don't see a bug in the examples you show :) so I'll try to explain one by one:

https://private-user-images.githubusercontent.com/7235289/320194824-f99f53bc-b856-4d40-8f64-37b89d9c240e.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTI0MDU0NzMsIm5iZiI6MTcxMjQwNTE3MywicGF0aCI6Ii83MjM1Mjg5LzMyMDE5NDgyNC1mOTlmNTNiYy1iODU2LTRkNDAtOGY2NC0zN2I4OWQ5YzI0MGUucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDQwNiUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA0MDZUMTIwNjEzWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9N2JhZjc3NmMxNWJhYmI4NmQ4YTkxYzAzY2U5NTE2MGQwM2RhMWZmY2ZkYTJlZGIwMmMxZjMwNTkzMDUyNjU4YSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.lHfchtNsBsV6Ptd6Adl-nFcPSzUzWl_Jr9JU8XEKrNQ
Look at t2: the message highlighted is NOT a potential-resource-leak, it is effective!

In the picture I see "Potential resource leak: "t2" may not be closed". Isn't that exactly what you are expecting? The message says potential.

Or did you mean to say "the leak is effective"? In that case, here's the explanation: t2 is acquired from the return value of calling Optional.get(). Without annotations the analysis has no way to see if the message receiver (an instance of Optional) will keep the reference to the resource and take care of closing it at a later point, or if calling get() is meant as transferring ownership / responsibility to the caller. That's why it says "potential", in order to signal to users: the analysis result is essentially inconclusive.

Here, too: how can this be a potential-resource-leak? It is quite obvious it is an effective one and no lambda is involved.

As you say: same issue, except that here the message receiver is not an instance but a class.

For further illustration: assume that the typical idiom would not be System.out.println() but System.getOut().println(). According to your reasoning every client calling System.getOut() would be forced to close the out stream. That would be wrong.

Without going deeper with lambdas & co. detectors are a mess when just methods are involved:

Please don't jump to conclusions 😄 - we have a non-trivial test suite demonstrating how it works. Each test case carefully crafted.

[mmariotti]

Or did you mean to say "the leak is effective"? In that case, here's the explanation: t2 is acquired from the return value of calling Optional.get(). Without annotations the analysis has no way to see if the message receiver (an instance of Optional) will keep the reference to the resource and take care of closing it at a later point, or if calling get() is meant as transferring ownership / responsibility to the caller. That's why it says "potential", in order to signal to users: the analysis result is essentially inconclusive.

I wasn't clear enough, I'll try again.

TestAutoCloseable t2 = Optional.ofNullable(t).orElseGet(() -> TestAutoCloseable.newInstance());
t2.toString();

The problem in this statement is not how the Closeable is obtained by/inside a method/lambda/expression. The problem is that an instance of Closeable is assigned to a local variable and that's never explicitly closed.
It's not important how the Closeable is obtained, when it is assigned (the very last operation of the line of code), it is open.
I'm not aware of a method, constructor, any way in general (that has no other serious problem), which returns a closed Closeable.

That said, we can safely assume that a Closeable assigned to a local variable is always open when the assignment occurs.

Let's focus on the local variable:

InputStream inputStream2 = Files.newInputStream(null);
InputStream inputStream3 = Channels.newInputStream((ReadableByteChannel) null);

A warning is reported for inputStream2, but only an info is reported for inputStream3.
This is the point.
There should be no difference, both are (open) Closeables assigned to local variables, both variables should be reported with warnings.
Please explain to me what is the reason for this difference because I can't really grasp it (actually, I think I know the reason, but I also think the reason is wrong).

As you say: same issue, except that here the message receiver is not an instance but a class.

I don't know what it means that the receiver is a class. The return value is just unassigned, I assume the receiver is simply "none".
In any other language without a GC this should probably be a leak (memory, handle, descriptor, ...) by itself (a resource is allocated but there's no pointer to access that resource - thus cannot be freed).

For further illustration: assume that the typical idiom would not be System.out.println() but System.getOut().println(). According to your reasoning every client calling System.getOut() would be forced to close the out stream. That would be wrong.

Are you sure it would be wrong?
Theoretically speaking, once the client code obtains an instance of sysout it should close it when finshed.
Nevertheless, in practice, we know that sysout is special and there's no need to close it.
Doesn't the same special treatment apply to Streams too? With only a few exceptions such as Streams returned by java.nio.Files methods.

Please don't jump to conclusions 😄 - we have a non-trivial test suite demonstrating how it works. Each test case carefully crafted.

I'm sorry, didn't mean to be rude.
But, honestly, I don't see a strong logic behind the different behaviors of the detector as shown in the last screenshot.
All of the statements are assignments of Closeables to local variables and they should be equivalent from the point of view of the detector.

[stephan-herrmann]

Clarification of terminology:

As you say: same issue, except that here the message receiver is not an instance but a class.

I don't know what it means that the receiver is a class.

Sorry, "receiver" is our (the compiler's) name for the thing you invoke a method on, i.e., the thing left of the dot in a method invocation (terminology leaked from when Eclipse was Visual Age, i.e., SmallTalk terminology, where a method invocation is call a "message send" - then "receiver" is who you send the message to).

[stephan-herrmann]

The problem in this statement is not how the Closeable is obtained by/inside a method/lambda/expression. The problem is that an instance of Closeable is assigned to a local variable and that's never explicitly closed.

When verbose explanations fail, perhaps code helps:

import java.util.zip.ZipEntry;
import java.util.zip.ZipFile;

class ZipCache {
	ZipFile get(String handle) {
		// FIXME: insert your logic to find a zip file by the given handle,
		// * either from a cache,
		// * or freshly read from disk (in that case it's added to the cache)
		return null; //  
	}
}
public class SharingDemo {
	ZipCache cache;
	void compare(String handle, String entry1, String entry2) {
		ZipEntry zEntry1 = cache.get(handle).getEntry(entry1);
		ZipEntry zEntry2 = cache.get(handle).getEntry(entry2);
		doCompare(zEntry1, zEntry2);
	}
	void doCompare(ZipEntry zEntry1, ZipEntry zEntry2) {
		if (zEntry1.getSize() > zEntry2.getSize())
			System.out.println("Larger: "+zEntry1.getName());
	}
}

In it's current form the program will compare sizes of two zip entries, given that class ZipCache has a reasonable implementation (should I explain further)?

If I understand your reasoning correctly, then focus is on local variable, so let's perfrom "Extract Local Variable" twice (not the smartest thing to do, but not a bug):

	void compare(String handle, String entry1, String entry2) {
		ZipFile zipFile1 = cache.get(handle);
		ZipEntry zEntry1 = zipFile1.getEntry(entry1);
		ZipFile zipFile2 = cache.get(handle);
		ZipEntry zEntry2 = zipFile2.getEntry(entry2);
		doCompare(zEntry1, zEntry2);
	}

You seem to want ecj to report a definite resource leak against each zipfile1 and zipfile2, so the user will add zipFile1.close() immediately after the local variable has been used (or the equivalent using t-w-r):

	void compare(String handle, String entry1, String entry2) {
		ZipFile zipFile1 = cache.get(handle);
		ZipEntry zEntry1 = zipFile1.getEntry(entry1);
		zipfile1.close();
		ZipFile zipFile2 = cache.get(handle);
		ZipEntry zEntry2 = zipFile2.getEntry(entry2);
		doCompare(zEntry1, zEntry2);
	}

Guess what: now the call zipFile2.getEntry(entry2) will throw IllegalArgumentException because it is called on a closed zip file :(

Admittedly, the example is contrived, but with just a bit of extra code the locations where zipFile1 and zipFile2 are used could be far away from each other. But even in this contrived example it would be very bad, if the compiler advises the user to add a close statement that will introduce a bug.