fix: properly close PackageManagers once all dependencies are retrieved

[netomi]

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.

[sschuberth]

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.

[codecov]

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]

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]

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]

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

[sschuberth]

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.

[netomi]

ok, changed.

[sschuberth]

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.

[netomi]

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

[sschuberth]

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

[netomi]

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