Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: properly close PackageManagers once all dependencies are retrieved #7980

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: properly close PackageManagers once all dependencies are retrieved #7980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion analyzer/src/main/kotlin/PackageManager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

package org.ossreviewtoolkit.analyzer

import java.io.Closeable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit message: I believe saying "properly close PackageManager instances" is misleading, as before this change package managers were not closeable, so you also couldn't close them.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, changed.

import java.io.File
import java.nio.file.FileSystems
import java.nio.file.Path
Expand Down Expand Up @@ -62,7 +63,7 @@ abstract class PackageManager(
val analysisRoot: File,
val analyzerConfig: AnalyzerConfiguration,
val repoConfig: RepositoryConfiguration
) {
) : Closeable {
companion object {
private val PACKAGE_MANAGER_DIRECTORIES = setOf(
// Ignore intermediate build system directories.
Expand Down Expand Up @@ -361,6 +362,11 @@ abstract class PackageManager(
}
}
}

/**
* [PackageManager] implementations should override this method if they need to close resources.
*/
override fun close() = Unit
}

/**
Expand Down
4 changes: 4 additions & 0 deletions plugins/commands/analyzer/src/main/kotlin/AnalyzerCommand.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,5 +228,9 @@ class AnalyzerCommand : OrtCommand(
val issues = analyzerRun.result.getAllIssues().flatMap { it.value }
SeverityStatsPrinter(terminal, resolutionProvider).stats(issues)
.print().conclude(ortConfig.severeIssueThreshold, 2)

for (packageManager in info.managedFiles.keys) {
packageManager.close()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I don't like so much about this approach is that every downstream user of Analyzer.analyze() has to call close() itself. I wonder whether a feasible / better approach would be for the respective package manager to override the existing afterResolution() and close their resources there.

Copy link
Author

@netomi netomi Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your point, the downside is that the Analyzer is fed with a a list of managedFiles with their associated PackageManager. Thus when the Analyzer would close the PackageManagers after it has finished analysis, the caller would not be able to use the package managers anymore in a safe way as they are already closed. This transfer of ownership would have to be documented imho, but I am indifferent where the closing should happen, as long it happens ofc :-)

}
}
}
5 changes: 5 additions & 0 deletions plugins/package-managers/gradle/src/main/kotlin/Gradle.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,4 +290,9 @@ class Gradle(
}
}
}

override fun close() {
// Silently close the [MvnSupport] instance.
maven.use {}
}
}
5 changes: 5 additions & 0 deletions plugins/package-managers/maven/src/main/kotlin/Maven.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,11 @@ class Maven(

return listOf(ProjectAnalyzerResult(project, emptySet(), issues))
}

override fun close() {
// Silently close the [MvnSupport] instance.
mvn.use {}
}
}

/**
Expand Down
7 changes: 6 additions & 1 deletion plugins/package-managers/maven/src/main/kotlin/utils/MavenSupport.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

package org.ossreviewtoolkit.plugins.packagemanagers.maven.utils

import java.io.Closeable
import java.io.File
import java.net.URI

Expand Down Expand Up @@ -108,7 +109,7 @@
private val File?.safePath: String
get() = this?.invariantSeparatorsPath ?: "<unknown file>"

class MavenSupport(private val workspaceReader: WorkspaceReader) {
class MavenSupport(private val workspaceReader: WorkspaceReader) : Closeable {
companion object {
private val PACKAGING_TYPES = setOf(
// Core packaging types, see https://maven.apache.org/pom.html#packaging.
Expand Down Expand Up @@ -814,6 +815,10 @@
legacySupport.session = null
}
}

override fun close() {
remoteArtifactCache.close()
}

Check warning on line 821 in plugins/package-managers/maven/src/main/kotlin/utils/MavenSupport.kt

View check run for this annotation

Codecov / codecov/patch

plugins/package-managers/maven/src/main/kotlin/utils/MavenSupport.kt#L820-L821 

Added lines #L820 - L821 were not covered by tests
}

/**
Expand Down
7 changes: 6 additions & 1 deletion utils/common/src/main/kotlin/DiskCache.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ package org.ossreviewtoolkit.utils.common

import com.jakewharton.disklrucache.DiskLruCache

import java.io.Closeable
import java.io.File
import java.io.IOException

Expand All @@ -46,7 +47,7 @@ class DiskCache(
* Duration in seconds that cache entries are valid.
*/
private val maxCacheEntryAgeInSeconds: Long
) {
) : Closeable {
companion object {
const val INDEX_FULL_KEY = 0
const val INDEX_TIMESTAMP = 1
Expand Down Expand Up @@ -144,4 +145,8 @@ class DiskCache(
}

private fun currentTimeInSeconds() = System.currentTimeMillis() / 1000L

override fun close() {
diskLruCache.close()
}
}