Memoizer fixes - #4464
Closed
Darlokt wants to merge 10 commits into
Closed
Conversation
The empty File path used for the implicit Memoizer cache is not a stable way to disable memoization: Java 25 resolves it as the current directory while older JDKs reject it as a cache directory. This made the standard data tests exercise different code paths depending on the JDK. Remove the implicit Memoizer wrapper from standard flattened-reader setup. Memoization remains covered by testMemoFileUsage, which already uses an isolated temporary cache directory.
Memo compatibility previously compared only the metadata level, so reader-specific DynamicMetadataOptions could reuse initialized state created with different pixel or metadata behavior. A current .bfoptions file was also not loaded before comparison, making its effective settings absent from the cache identity. Give metadata option implementations value equality, compare the complete option state in FormatTools.equalReaders, and load the current .bfoptions before Memoizer accepts a cached reader. Cover custom option changes and unchanged/changed option files with cache-level regression tests.
Memoizer previously compared the memo timestamp only with the primary identifier. Multi-file datasets could therefore restore stale initialized state after a companion metadata file, pixel file, or .bfoptions file changed. Version the memo format and record a manifest containing a relocatable path, existence flag, size, and modification time for every getUsedFiles entry. Verify the manifest before deserializing the reader, while retaining support for moving a dataset and its in-place memo together. Add an unchanged/changed FakeReader companion-file regression test.
File.renameTo returned only a boolean and Memoizer logged a false result without clearing its success flag. Callers could consequently observe saveMemo and isSavedToMemo as true even though the final memo file did not exist. Install completed temporary memos with a checked atomic Files.move, falling back to a replace move when atomic moves are unsupported. Propagate installation failure into the returned status and cover the failure path with an injected move error.
Configured cache directories stripped the root from absolute identifiers. On Windows this also stripped the drive letter, so files such as C:\somedir\foo.nd2 and D:\somedir\foo.nd2 mapped to the same memo and could restore state for the wrong dataset. Prefix drive-based cache paths with the normalized drive letter and UNC paths with their server/share hierarchy, while preserving the existing Unix layout. Add platform-independent drive and UNC mapping tests. Fixes ome#3034.
Extend memo manifests with dependency-directory membership so adding or removing companion files invalidates stale reader state. Use versioned manifests, stable temporary-file names, and checked directory fingerprints while preserving relocatable file paths. Document metadata-option equality requirements and cover value-based and identity-based options, same-size replacements, added/deleted companions, failed memo installation, temporary artifacts, and Windows volume identity.
Author
|
b91d148 adds documentation and improved best-effort companion file checks as well as improved testing coverage. |
Use try-with-resources for every Memoizer instance in MemoizerTest so wrapped readers are closed when setup or assertions fail. Preserve explicit close-and-reopen calls in tests that exercise reader reuse. Update version-checking documentation for the dependency-manifest memo format and remove the unreachable legacy revision-check branch.
Author
|
As in #4463 this now properly uses the closeable. |
Track every temporary directory as soon as it is created and remove complete cache trees during always-run teardown. Restore directory writability before deletion so permission tests do not leave fixtures behind. Close wrapper and integration-test Memoizer instances on exceptional paths, and report cleanup failures instead of silently ignoring failed deletions. Guard directory listings explicitly before inspecting memo artifacts. Cover permission, wrapper, and failed memo-install cleanup paths with focused tests.
This was referenced Aug 5, 2026
Member
|
We are not able to accept these changes at this time. Any changes to memoization have serious implications for OMERO and other tools, and require a great deal of care during implementation, review, and testing. The extensive set of changes here, including changes to the memo file format itself, are not in line with our current priorities. They also conflict with ongoing work on #4254. We further note that this is one of several large-scale changes made without prior communication with the OME team. We strongly encourage you to start a discussion on a relevant issue prior to making such changes. |
|
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/new-bioformats-cv7000-reader-fixing-old-one-quietly-inventing-data/122100/1 |
This was referenced Aug 8, 2026
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
3 participants
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.
Hej,
While testing Bio-Formats memoization with the Java 25 build from #4450, several independent memoization problems surfaced. I did not take care of my branch and now its quite stacked, so this combines all the fixes here. The
MetadataOnlyschema issue is now being addressed upstream in ome/ome-model#233, so that workaround from #4459 is intentionally excluded here.Changes
Keep standard data tests on direct readers
Avoid implicit memoization in standard data tests. An empty cache path behaves differently across Java versions, including Java 25.
Fixed in aaa50ef.
Invalidate memos when effective reader options change
Compare complete metadata-option state and load the current
.bfoptionsfile before accepting a cached reader.Fixed in 39a7fe1.
Invalidate memos when any used file changes
Store a versioned manifest for every
getUsedFiles()entry, including relocatable paths, existence, size, and modification time. Reject stale memos when any dependency changes.Fixed in 96f3128.
Report memo saves only after successful installation
Install temporary memo files atomically where supported, fall back when necessary, and correctly report installation failures through
saveMemo()andisSavedToMemo().Fixed in 0a38ed2.
Keep Windows volume identity in memo cache paths
Preserve Windows drive letters and UNC share names so different volumes cannot map to the same memo.
Fixes #3034
Fixed in 787e398.
Fix memo cache path expectations on Windows
Update regression tests for platform-independent drive and UNC cache-path behavior.
Fixed in d126395.
Tests
Add focused coverage for metadata-option changes,
.bfoptionschanges, companion-file invalidation, relocation, failed installation, and Windows drive/UNC paths.The normal focused
MemoizerTestexecution passes. The configuredtest-no-hdfexecution still reports twoMemoizer(0)save failures, but this is not related to these changes.