Fix resource leaks #591

KengoTODA · 2018-03-21T03:32:49Z

Current implementation does not close SourceFinder even after analysis.
This PR also applies try-with-resources to handle zip files.

@iloveeclipse I'm not sure about the difference between FindBugs2#dispose() and FindBugs2#clearCache(), could you review? At least, can we destruct project instance by clearCaches() or not?

KengoTODA · 2018-03-29T10:42:19Z

This PR still needs to add a fix for PluginLoader, see roberthubert@7c47138

On Windows, cached connection can keep file handler then it causes resource leak. We have no motivation to use cached connection so we disable cache by default. This commit is cherry-pick from roberthubert@7c47138

spotbugs-bot · 2018-03-30T03:33:52Z

spotbugs/src/main/java/edu/umd/cs/findbugs/Project.java

-    public static Project readXML(File f) throws IOException,  SAXException {
-        InputStream in = new BufferedInputStream(new FileInputStream(f));
+    public static Project readXML(File f) throws IOException, SAXException {
+        @SuppressWarnings("resource") // will be closed by caller
        Project project = new Project();


Use try-with-resources or close this "Project" in a "finally" clause.

…-leaks

spotbugs-bot · 2018-04-08T09:16:50Z

test-harness-core/src/main/java/edu/umd/cs/findbugs/test/AnalysisRunner.java

+    }
+
+    @CheckReturnValue
+    private Project createProject(Path[] files) {
        final Project project = new Project();


Use try-with-resources or close this "Project" in a "finally" clause.

victorwss · 2018-04-10T22:22:39Z

Other issues targeted for 3.1.3, like #593/#602 were already fixed. If I understood it right, it seems that 3.1.3 was not yet released because it should contain the fix for this issue.

However, #593/#602 is a blocker for me, so I'm really needing a new release ASAP. Then, I would like to know if this issue is in a state to be fixed and merged very soon or not. If not, I suggest to defer it to 3.1.4 and release 3.1.3 with what we already have.

KengoTODA · 2018-04-28T11:07:09Z

CHANGELOG.md

@@ -15,6 +15,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 * Handle annotation on `package-info.class` properly ([#592](https://github.com/spotbugs/spotbugs/issues/592))
 * Update asm to 6.1.1 to support Java 10
 * Update Apache BCEL to 6.2 to support Java 9 package & module reference
+* Close source file after analysis ([#591](https://github.com/spotbugs/spotbugs/issues/591))


I'll move this to Unreleased part

…-leaks

KengoTODA · 2018-04-30T13:09:22Z

@iloveeclipse please check and review:

@iloveeclipse I'm not sure about the difference between FindBugs2#dispose() and FindBugs2#clearCache(), could you review? At least, can we destruct project instance by clearCaches() or not?

Note that we can ignore these two SonarQube warnings; method caller should be responsible to close resources, not these methods themselves.

iloveeclipse · 2018-04-30T13:33:27Z

If I remember right, clearCaches() was there to allow Eclipse plugin to free memory after analyis while the rest of FB2 would still be reusable. dispose() is the end of life. I quickly tried the change, it seem no to break the plugin, but I have no time for a deep review. I would probably don't close the project on clearCaches() call, it seem to close the source finder which seem to close all source repositories, and they will be not restored again.

KengoTODA · 2018-05-09T07:00:29Z

OK then I will stop closing project in clearCaches(), and make sure that FindBugs2.main(...) invokes not only clearCaches() but also dispose().

…2.main()

…eaks

spotbugs-bot · 2018-05-13T02:45:49Z

spotbugs/src/main/java/edu/umd/cs/findbugs/BugRanker.java

@@ -137,7 +138,7 @@ void storeAdjustment(String key, String value) {
        if (u == null) {
            return;
        }
-        BufferedReader in = UTF8.bufferedReader(u.openStream());
+        BufferedReader in = UTF8.bufferedReader(IO.openNonCachedStream(u));
        try {
            while (true) {


Reduce the total number of break and continue statements in this loop to use at most one.

spotbugs-bot · 2018-05-13T02:45:50Z

spotbugs/src/main/java/edu/umd/cs/findbugs/io/IO.java

 import java.net.URL;
 import java.net.URLConnection;

 import javax.annotation.CheckForNull;
 import javax.annotation.WillClose;
 import javax.annotation.WillNotClose;

+import edu.umd.cs.findbugs.annotations.CheckReturnValue;
+import edu.umd.cs.findbugs.annotations.NonNull;
 import edu.umd.cs.findbugs.util.Util;

 public class IO {


Add a private constructor to hide the implicit public one.

spotbugs-bot · 2018-05-13T02:45:50Z

spotbugs/src/main/java/edu/umd/cs/findbugs/PluginLoader.java

@@ -267,8 +267,8 @@ private static void finishLazyInitialization()  {
                System.err.println(msg);


Replace this use of System.out or System.err by a logger.

spotbugs-bot · 2018-05-13T02:45:51Z

spotbugs/src/main/java/edu/umd/cs/findbugs/PluginLoader.java

@@ -1623,8 +1614,6 @@ public static Summary validate(File file) throws IllegalArgumentException {
            throw new IllegalArgumentException(e);
        } catch (IOException e) {


Combine this catch with the one at line 1613, which has the same body.

spotbugs-bot · 2018-05-13T02:45:51Z

test-harness-core/src/main/java/edu/umd/cs/findbugs/test/AnalysisRunner.java

+            if (!bugReporter.getQueuedErrors().isEmpty()) {
+                AssertionError assertionError = new AssertionError(
+                        "Analysis failed with exception. Check stderr for detail.");
+                bugReporter.getQueuedErrors().stream().map(error -> error.getCause())


Replace this lambda with a method reference.

spotbugs-bot · 2018-05-13T02:45:52Z

test-harness-core/src/main/java/edu/umd/cs/findbugs/test/AnalysisRunner.java

        final Project project = new Project();
        project.setProjectName(getClass().getSimpleName());
-        engine.setProject(project);
-
        if (PLUGIN_JAR != null) {
            try {
                String pluginId = Plugin.addCustomPlugin(PLUGIN_JAR.toURI()).getPluginId();


A "NullPointerException" could be thrown; "addCustomPlugin()" can return null.

spotbugs-bot · 2018-05-13T02:45:52Z

spotbugs/src/main/java/edu/umd/cs/findbugs/workflow/RejarClassesForAnalysis.java

@@ -648,17 +648,14 @@ boolean processZipEntries(File f, ZipElementHandler handler) {
            System.out.println("empty zip file: '" + f + "'");


Replace this use of System.out or System.err by a logger.

spotbugs-bot · 2018-05-13T02:45:53Z

SonarQube analysis reported 254 issues

3 blocker
39 critical
133 major
69 minor
10 info

Watch the comments in this conversation to review them.

Top 10 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

FindBugs2.java#L123: Rename field "progress" to prevent any misunderstanding/clash with field "PROGRESS" defined on line 92.
BugRanker.java#L213: Refactor this method to reduce its Cognitive Complexity from 18 to the 15 allowed.
BugRanker.java#L301: Refactor this method to reduce its Cognitive Complexity from 18 to the 15 allowed.
FindBugs2.java#L189: Refactor this method to reduce its Cognitive Complexity from 26 to the 15 allowed.
FindBugs2.java#L476: Refactor this method to reduce its Cognitive Complexity from 31 to the 15 allowed.
FindBugs2.java#L486: Define a constant instead of duplicating this literal "Unable to read filter: " 3 times.
FindBugs2.java#L704: Refactor this method to reduce its Cognitive Complexity from 42 to the 15 allowed.
FindBugs2.java#L948: Refactor this method to reduce its Cognitive Complexity from 96 to the 15 allowed.
PluginLoader.java#L231: Refactor this method to reduce its Cognitive Complexity from 17 to the 15 allowed.
PluginLoader.java#L303: Change this "try" to a try-with-resources.

KengoTODA · 2018-05-13T02:57:55Z

@iloveeclipse PR has succeeded. Please check. I think all the warning by spotbugs can be ignored, since they're for existing code, not the newly added one.

Please use 'squash and merge' to merge this PR, it seems that recently GitHub isn't so wise to handle conflict.

iloveeclipse

LGTM

KengoTODA added 5 commits March 19, 2018 11:28

support not only Closeable but also AutoCloseable

0e1c0e2

close SourceFinder instance after analysis

1409743

close ZipFile instances certainly by try-with-resources

ae6b541

update CHANGELOG

8041e0f

close project and classpath before dereference them

ba89bc8

KengoTODA added the enhancement label Mar 21, 2018

KengoTODA self-assigned this Mar 21, 2018

KengoTODA requested a review from iloveeclipse March 21, 2018 03:32

refs #591: add comment to explain why close() does nothing

2baa5d1

KengoTODA changed the title ~~Fix resource leaks~~ WIP: Fix resource leaks Mar 21, 2018

This was referenced Mar 28, 2018

Plugin file handles are leaking #589

Closed

File handle to findsecbugs-plugin.jar is leaking spotbugs/sonar-findbugs#128

Closed

refs #591: Use non-cached connection for resources loaded from URL.

7c9ac82

On Windows, cached connection can keep file handler then it causes resource leak. We have no motivation to use cached connection so we disable cache by default. This commit is cherry-pick from roberthubert@7c47138

KengoTODA referenced this pull request in roberthubert/spotbugs Mar 30, 2018

Fixed leaking file handle on custom plugin removal

7c47138

refs #591: comment why we do not use try-with-resources

6b1814f

spotbugs-bot reviewed Mar 30, 2018

View reviewed changes

Merge remote-tracking branch 'spotbugs/release-3.1' into fix-resource…

1bb6c30

…-leaks

KengoTODA added this to the SpotBugs 3.1.3 milestone Apr 2, 2018

close resources after unit test

659e03e

spotbugs-bot reviewed Apr 8, 2018

View reviewed changes

Merge branch 'release-3.1' into fix-resource-leaks

fb1a4f7

KengoTODA modified the milestones: SpotBugs 3.1.3, SpotBugs 3.1.4 Apr 18, 2018

svilenvul mentioned this pull request Apr 27, 2018

Resource leak ? Maven build fails with "Too many open files in system" openhab/static-code-analysis#258

Closed

KengoTODA commented Apr 28, 2018

View reviewed changes

KengoTODA and others added 3 commits April 28, 2018 19:10

Merge remote-tracking branch 'spotbugs/release-3.1' into fix-resource…

aa342e3

…-leaks

update CHANGELOG

319e10c

Merge branch 'release-3.1' into fix-resource-leaks

c39cfeb

KengoTODA changed the title ~~WIP: Fix resource leaks~~ Fix resource leaks Apr 30, 2018

KengoTODA added 2 commits May 9, 2018 15:02

refs #591: call not only clearCaches() but also dispose() in FindBugs…

b65d57e

…2.main()

Merge remote-tracking branch 'origin/release-3.1' into fix-resource-l…

5585398

…eaks

spotbugs-bot reviewed May 13, 2018

View reviewed changes

iloveeclipse approved these changes May 16, 2018

View reviewed changes

KengoTODA merged commit 5f88794 into release-3.1 May 18, 2018

KengoTODA deleted the fix-resource-leaks branch May 18, 2018 11:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix resource leaks #591

Fix resource leaks #591

KengoTODA commented Mar 21, 2018

KengoTODA commented Mar 29, 2018

spotbugs-bot Mar 30, 2018

spotbugs-bot Apr 8, 2018

victorwss commented Apr 10, 2018

KengoTODA Apr 28, 2018

KengoTODA commented Apr 30, 2018

iloveeclipse commented Apr 30, 2018 •

edited

Loading

KengoTODA commented May 9, 2018

spotbugs-bot May 13, 2018

spotbugs-bot May 13, 2018

spotbugs-bot May 13, 2018

spotbugs-bot May 13, 2018

spotbugs-bot May 13, 2018

spotbugs-bot May 13, 2018

spotbugs-bot May 13, 2018

spotbugs-bot commented May 13, 2018

KengoTODA commented May 13, 2018

iloveeclipse left a comment

		@@ -267,8 +267,8 @@ private static void finishLazyInitialization() {
		System.err.println(msg);

		@@ -1623,8 +1614,6 @@ public static Summary validate(File file) throws IllegalArgumentException {
		throw new IllegalArgumentException(e);
		} catch (IOException e) {

		@@ -648,17 +648,14 @@ boolean processZipEntries(File f, ZipElementHandler handler) {
		System.out.println("empty zip file: '" + f + "'");

Fix resource leaks #591

Fix resource leaks #591

Conversation

KengoTODA commented Mar 21, 2018

KengoTODA commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorwss commented Apr 10, 2018

Choose a reason for hiding this comment

KengoTODA commented Apr 30, 2018

iloveeclipse commented Apr 30, 2018 • edited Loading

KengoTODA commented May 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spotbugs-bot commented May 13, 2018

Top 10 extra issues

KengoTODA commented May 13, 2018

iloveeclipse left a comment

Choose a reason for hiding this comment

iloveeclipse commented Apr 30, 2018 •

edited

Loading