Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 53 |
| Duplication | 8 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
On the SonarCloud duplication gate (6.5% > 3%): The duplication is an inherent consequence of the multi-release jar approach used here, not an oversight. src/main/java22/.../LongMappedByteBuffer.java is a brand-new file that necessarily re-implements the same public API as the Java 21 LongMappedByteBuffer — that's the whole point of a multi-release jar: two classes with an identical contract, chosen by JVM version at load time. A handful of trivial delegating methods (get(), get(byte[], int, int), put(byte), read(byte[], int, int), position(), position(long), size(), limit()) end up textually identical between the two versions because their behavior is version-independent — only the low-level primitives they delegate to (get(long), get(long, byte[], int, int), put(long, byte), close(), …) differ, and those are exactly the FFM-based parts. On top of that, the new file carries the project's standard ~49-line MPL license header, which — being a brand-new file — SonarCloud counts entirely as "new code" and matches against the same header present in effectively every other file in the codebase. Without raising the Java 21 baseline (explicitly out of scope per #1439) or dropping the deterministic-unmap fix for Java 22+, I don't see a way to avoid this class-level duplication — the two implementations must exist as separate classes for the multi-release jar mechanism to work at all. Happy to factor the ~8 identical delegating methods out into a small package-private shared base class if that's preferred, but that only removes a part of the flagged lines (the license header duplication would remain), and it's a structural change I'd rather make deliberately if you'd like to see it, rather than as a reflex to the gate. Let me know how you'd like to proceed — including whether an exclusion for this file on the SonarCloud project side would be the preferred route. |
|


Description of the new Feature/Bugfix
Since the switch to
LongMappedByteBuffer(#1319),MappedRandomAccessFile.close()only closes theFileChannel.The mapping itself stays alive until the
MappedByteBufferchunks are garbage collected. On Windows, a mapped fileis locked, so it cannot be deleted or moved after
PdfReader.close()(#1112, #1517). The only workaround today isSystem.gc(), which is also whatSmallPdfReadTestdoes.In #1439, restoring the
Unsafebased cleaner was declined, and the FFM API (JEP 454) was named as the properlong-term solution. This PR implements exactly that, without raising the Java 21 baseline, by using a
multi-release jar (the parent POM already declares
Multi-Release: true):src/main/java22/org/openpdf/text/utils/LongMappedByteBuffer.java(new, used on Java 22+): maps the file into asingle
MemorySegmentwithFileChannel.map(mode, 0, size, Arena.ofShared()).close()closes the arena, whichunmaps the file immediately. The public API is identical to the Java 21 class (checked with
jar --validate),including the exceptions (
BufferUnderflowExceptionat EOF,ReadOnlyBufferExceptionfor read-only mappings).A shared arena is used because a
PdfReadermay be created and read in different threads. Access afterclose()throws
IllegalStateException; unmapped memory is never touched. Only supported, final Java APIs are used — noUnsafe, no reflection, no--add-opens.LongMappedByteBuffer(Java 21): implementsAutoCloseable;close()drops the chunk references. Behavior onJava 21 is otherwise unchanged.
MappedRandomAccessFile:close()now closes the buffer (so the Javadoc "Cleans the mapped bytebuffer" is trueagain). The channel is also closed if mapping fails in the constructor; previously the file handle leaked.
openpdf-core/pom.xml: profilemulti-release-java22, active on JDK 22+. It compilessrc/main/java22intoMETA-INF/versions/22and runs the relevant tests with failsafe against the packaged jar (surefire usestarget/classes, where the JVM ignoresMETA-INF/versions). With JDK 21 the build is unchanged.Related Issue: #1112, #1517 (see also #1439)
Unit-Tests for the new Feature/Bugfix
Unit-Tests added to reproduce the bug
Unit-Tests added to the added feature
LongMappedByteBufferTest: the contract both implementations must fulfil (EOF handling, bulk reads, bounds,empty files, read-only and read-write mappings, idempotent
close()). Runs with surefire (Java 21 classes) and,on JDK 22+, again with failsafe against the multi-release jar.
MappedFileReleaseTest: verifies that the file is released right afterclose()— forMappedRandomAccessFile,PdfReader(String)and a partialPdfReader(RandomAccessFileOrArray, ...)— deliberately withoutSystem.gc(). On Windows viaFiles.delete(), on Linux via/proc/self/maps. Skipped on Java < 22.Compatibilities Issues
LongMappedByteBuffer implements AutoCloseablewith a newclose().LongMappedByteBufferafterclose()throwsIllegalStateException(before: it kept workingor failed with a
NullPointerException, depending on the caller).variant only. Maybe worth an enforcer rule in the
releaseprofile — happy to add it if you prefer.Your real name
Christian Voss
Testing details
Tested locally on Windows 11 (the platform #1112/#1517 are actually about) with Temurin 21.0.12 and Temurin
25.0.4, via
mvnw -B clean install -pl openpdf-core:LongMappedByteBufferTest+SmallPdfReadTest(surefire)MappedFileReleaseTest+LongMappedByteBufferTest(failsafe, JDK 22+ only)Full reactor run (
openpdf-core): 2089 tests, 0 failures on both JDKs (49.9 s on JDK 21, 54.4 s wall clock onJDK 25).
Bug reproduction on Windows. To confirm
MappedFileReleaseTestactually detects the bug (and isn't just green byconstruction), I re-ran the failsafe execution with
classesDirectorypointed attarget/classesinstead of thepackaged jar — i.e. the same code path a plain classpath run (or a build without the multi-release jar) would take,
since a directory on the classpath doesn't get the
META-INF/versionstreatment that a real jar gets. Result: 1failure + 3 errors, all
java.nio.file.FileSystemException:("The process cannot access the file because it is being used by another process" — the exact symptom in #1112 and
#1517.) With
classesDirectoryrestored to the jar, the same 4 tests pass.pom.xmlwas reverted immediately after(
git statusclean, no diff left behind).Other checks:
jar tf openpdf-core/target/openpdf-3.0.6-SNAPSHOT.jar | findstr versions→ containsMETA-INF/versions/22/org/openpdf/text/utils/LongMappedByteBuffer.class.jar --validate --file openpdf-core/target/openpdf-3.0.6-SNAPSHOT.jar→ no output, exit code 0.mvnw checkstyle:check -pl openpdf-core→ "You have 0 Checkstyle violations." (checkstyle 13.5.0, project config).Performance, Windows 11, 256 MB file, via
MappedRandomAccessFile(single run, ad-hoc benchmark, not JMH — seebelow for caveats):
MappedByteBuffer, before)close()FileSystemException, file still locked)Notes on the performance numbers: single run on a dev laptop, JIT warmup not isolated, no JMH — treat as a rough,
directional, Windows-specific data point rather than a rigorous benchmark. Two things stood out compared to the
Linux numbers quoted in earlier drafts of this PR: on this Windows machine the FFM path was not slower for
open+close (it was faster, likely because it avoids holding a Windows file handle open until GC), and random reads
were slightly slower with FFM (~+24%) — plausibly a JIT/memory-segment-bounds-check effect rather than something
inherent to the approach. Reviewers who care about the random-read cost may want to re-check with a proper
benchmark harness before relying on these numbers.
The
open + close/delete after close()row is the one that matters for #1112/#1517: on JDK 21, deleting the256 MB file (and, separately, the small 4 KiB file used in the open/close loop) right after
close()fails withFileSystemExceptionevery time on Windows; on JDK 25 with the multi-release jar it succeeds every time — with noSystem.gc()anywhere in the benchmark.To reproduce the bug directly on Windows: remove
System.gc()/Thread.sleep(100)fromSmallPdfReadTest, or runMappedFileReleaseTestwith failsafe'sclassesDirectorypointed attarget/classesinstead of the jar (see above).Note on AI assistance
This PR was prepared with AI assistance: Claude (Anthropic) drafted the initial patch and description and I used it to verify it — rebuilding and running the full test suite on Windows with both JDKs, reproducing the underlying bug independently (by pointing
classesDirectoryattarget/classes), and running the ad-hoc benchmark above. All results in this description reflect actual local runs, not generated text. I've reviewed the code changes and take responsibility for this contribution.