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.

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

Conversation

netomi
Copy link

@netomi netomi commented Dec 1, 2023

This fixes #7978 .

This PR makes PackageManagers implement the Closeable interface to be able to close their resources once they are not used anymore (at the end of the analyzer run).

There is a default implementation that does nothing, the maven / gradle package manager override this method in order to close the DiskCache they are using via the MavenSupport class.

@netomi netomi requested a review from a team as a code owner December 1, 2023 21:25
@sschuberth
Copy link
Member

Thanks for spotting the issue and proposing a solution! To get started, there are a bunch of formal issues with this PR. Please do:

  • Adhere to Conventional Commits.
  • Address review comments / fixes to existing changes in this PR as fixups to the existing commits.
  • Imports need to be sorted strictly alphabetically, with a single empty line between import from different top-level packages.

@netomi netomi force-pushed the close-disklru-cache branch 2 times, most recently from 33b6b62 to 2e145e9 Compare December 1, 2023 21:51
@netomi netomi changed the title Make PackageManagers Closeable fix: Properly close PackageManagers once all dependencies are retrieved Dec 1, 2023
@netomi netomi changed the title fix: Properly close PackageManagers once all dependencies are retrieved fix: properly close PackageManagers once all dependencies are retrieved Dec 1, 2023
Copy link

codecov bot commented Dec 1, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (5af7043) 67.01% compared to head (4081a72) 66.93%.
Report is 10 commits behind head on main.

Files Patch % Lines
...nagers/maven/src/main/kotlin/utils/MavenSupport.kt 33.33% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7980      +/-   ##
============================================
- Coverage     67.01%   66.93%   -0.09%     
- Complexity     2041     2049       +8     
============================================
  Files           357      357              
  Lines         17103    17083      -20     
  Branches       2443     2455      +12     
============================================
- Hits          11462    11434      -28     
- Misses         4623     4627       +4     
- Partials       1018     1022       +4     
Flag Coverage Δ
funTest-docker 65.95% <33.33%> (-0.97%) ⬇️
funTest-non-docker 34.35% <ø> (ø)
test 36.29% <0.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fviernau
Copy link
Member

fviernau commented Dec 4, 2023

Thanks @netomi for providing this fix.
After reading up on the comments of the GitHub issue, it seems that non-atomic reads to the journal
a suspected to be the root cause, see [1]. Assuming this is the case, I do not understand how / why this PR is supposed to fix the issue.

@netomi do you have any explanation why this fix does work?
Can you share the reproducibility rate you achived before making this fix? e.g. difficult it was to reproduce, and also how you verified the fix works?

[1] JakeWharton/DiskLruCache#52 (comment)

@netomi
Copy link
Author

netomi commented Dec 4, 2023

I noticed that problem when running ort multiple times locally. Of course randomly but something like every 2-3 runs, the cache would have been corrupted.

I found this ticket as it sounds very much like the same problem. The journalWriter is a wrapped BufferedWriter, and not closing that writer (or at least flushing its contents) might explain the problem. When the cache was corrupted it looked like that the last entry was not completely written to the file, so that might be the most likely explanation in this case.

Closing resources after they are not used anymore is good practice anyways so I implemented this change. After that change I did not notice a corrupted cache anymore, e.g. I did like 10 run of ORT in a row without noticing the problem anymore.

Am I sure that this fixes the problem? No, but I am confident that this fix improves the situation and also gives other PackageManager implementations the chance to close resources before terminating.

@fviernau
Copy link
Member

fviernau commented Dec 4, 2023

@netomi Quite a while back, we had an issue in ORT that even after the termination of the CLI, ORT's JVM process kept on running. (IIRC this happened due to some code path not existing the thread after catching (or not catching) some exception).

Just in case we should have a similar problem again, that could be as well the root cause of the problem.
It would be great if you could reproduce the issue again, and see if ORT's process kept on running even after CLI exit?!

@netomi netomi force-pushed the close-disklru-cache branch 2 times, most recently from ff1323e to ba906db Compare December 4, 2023 16:40
@@ -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.

@@ -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

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 :-)

@@ -0,0 +1,20 @@
<?xml version='1.0' encoding='UTF-8'?>
Copy link
Member

Choose a reason for hiding this comment

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

The files committed here and below seem like temporary left-overs.

Copy link
Author

Choose a reason for hiding this comment

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

indeed, using git add -A is not really a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DiskCache used for maven project gets corrupted sometimes
3 participants