ort icon indicating copy to clipboard operation
ort copied to clipboard

fix: properly close PackageManagers once all dependencies are retrieved

Open netomi opened this issue 1 year ago • 5 comments

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 avatar Dec 01 '23 21:12 netomi

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.

sschuberth avatar Dec 01 '23 21:12 sschuberth

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.

codecov[bot] avatar Dec 01 '23 21:12 codecov[bot]

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

fviernau avatar Dec 04 '23 07:12 fviernau

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 avatar Dec 04 '23 07:12 netomi

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

fviernau avatar Dec 04 '23 08:12 fviernau