Simplify ArchiveManager archived entry cache#1022
Open
bertm wants to merge 6 commits intohyphanet:nextfrom
Open
Simplify ArchiveManager archived entry cache#1022bertm wants to merge 6 commits intohyphanet:nextfrom
bertm wants to merge 6 commits intohyphanet:nextfrom
Conversation
The `lastSize` and `lastHash` on the ArchiveStoreContext appear to keep track of the last observed size and hash for an archive data bucket, but they are only ever written by the ArchiveManager when they already have a value - which they never do. Therefore, there are no code paths for ArchiveRestartedException to be thrown. This has been the case for at least since 2005, and apparently it works fine this way. Inline the default values of these fields in ArchiveManager and prune the now unreachable code.
Remove the double bookkeeping of archive entries in ArchiveManager and ArchiveStoreContext and replace it with a central cache implementation. This simplifies the locking (for which there were instructions in the comments, but these were ignored in the implementation) and reduces the number of arguments being passed around. This also removes the "remove all cached items for an archive" logic that would likely lead to memory leaks as not all references were consistently released, and fixes some potential race conditions in claiming the buckets.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
ArchiveManageris keeping track of cached entries in all kinds of different ways, with a lot of code paths being effectively unused.ArchiveRestartExceptionshould be thrown for the above reason, but there is no code path where this can actually happen.nullresult when queried).FreenetURIas a cache key, which is mutable and could break the cache when changed.Move the cache implementation to a dedicated class that has proper unit tests, and remove all the code paths that were effectively no-op (ignoring a few log statements).
I would suggest to review this commit by commit. The first two commits contain the bulk of the work (their commit messages explain what is happening), the remaining four commit are simpler after the fact cleanups.