ort
ort copied to clipboard
fix: properly close PackageManagers once all dependencies are retrieved
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.
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 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 :warning: |
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%) |
:arrow_down: |
| funTest-non-docker | 34.35% <ø> (ø) |
|
| test | 36.29% <0.00%> (+0.02%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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] https://github.com/JakeWharton/DiskLruCache/issues/52#issuecomment-1500755265
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.
@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?!