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 disk cache cleanup sort errors caused by files being altered during sorting #1238

Merged
merged 6 commits into from
Sep 26, 2023

Conversation

okey
Copy link
Contributor

@okey okey commented Sep 22, 2023

Description of Changes

Java versions newer than 6 check that the sort invariant is maintained. If a cached file is modified during sort, then the modified time changes, violating the invariant and causing an exception to be thrown. Previously, this would have silently caused the resulting sort to be incorrect.

Neither of these results are ideal, but throwing an exception causes the cache cleanup to fail, which can lead to unexpectedly high cache usage.

A quick and nasty hack to revert to Java 6 behaviour is to insert System.setProperty("java.util.Arrays.useLegacyMergeSort", "true"); somewhere, but it is better to fix the sort completely.

This PR caches the file modification times and looks them up during the sort.

PR Checklist

  • Link to any issues that the PR addresses
  • Add labels
  • Open as a draft PR
    until ready for review
  • Make sure GitHub tests pass
  • Mark PR as "Ready for Review"

@CLAassistant
Copy link

CLAassistant commented Sep 22, 2023

CLA assistant check
All committers have signed the CLA.

@okey okey marked this pull request as ready for review September 25, 2023 18:52
@haileyajohnson
Copy link
Contributor

Thanks for the contribution, @okey! I refactored your solution a bit to still plug in a comparator, but one that uses a static lastModified property. As for cleanCache(long maxBytes, Comparator<File> fileComparator, StringBuilder sbuff), I don't think we want to deprecate it since there are still reasons users may want to sub their own Comparator to sort the files in their cache.

@okey
Copy link
Contributor Author

okey commented Sep 26, 2023

@haileyajohnson Thanks for the refactor, if you're happy with using a static property then I think it's a good solution.
Do you want to fix the test failures or shall I?

@haileyajohnson haileyajohnson merged commit 2344f87 into Unidata:maint-5.x Sep 26, 2023
9 checks passed
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.

4 participants