Fix/gh 51463 nested jar locking - #51580
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
| throw new IllegalStateException("Zip file closed"); | ||
| } | ||
| if (this.resources.zipContent() == null) { | ||
| ZipContent zipContent = this.resources.zipContent(); |
There was a problem hiding this comment.
While validating this approach I realised that the zipContent variable being returned from the zipContent() method isn't a volatile field so there is a small gap here. Adding volatile to this field in NestedJarFileResources would close the gap. Without changing it to volatile though the code should still be safe as the reference counting in the FileDataBlock would catch it and throw a consistent error (no corruption or deadlock).
| } | ||
|
|
||
| <E extends Exception> void ensureOpen(Supplier<E> exceptionSupplier) throws E { | ||
| synchronized (this.lock) { |
There was a problem hiding this comment.
I will need to find or reproduce the thread dump to confirm but I believe this was where the blocking moved to after sorting the NestedJarFile concurrency. There were 4 places inside the class competing for the same lock. The open, close, read and ensureOpen. The only usage of ensureOpen is in the same method call as the read. This change removes the synchronisation entirely from ensureOpen by using an AtomicInteger for the reference tracking which reduces internal contention. Previously FileDataBlock#read was needing to synchronize on the lock twice for each call.
There was a problem hiding this comment.
Changed approach - reverted my original change as i didn't want to include it but then found that there was a gap in the read method meaning that the ensureOpen result could be stale by the time the read actually occurred and the read didn't recheck that it was still open in the sync block. Have moved referenceCount to be volatil so the ensureOpen no longer needs a sync block as it is informative only and the read now also checks the referenceCount within the same sync block that actually does the read
…edJarFile NestedJarFile exposes its monitor via synchronized(this), allowing external code to acquire it directly. This creates a classic AB-BA deadlock: one thread holds an unrelated lock (e.g., ClassLoader or reflection machinery) while waiting on NestedJarFile's monitor, while another thread holds that monitor while waiting on the unrelated lock. Replace synchronized(this) with a private final Object mutex throughout NestedJarFile, except in close() where super.close() synchronizes on 'this' internally. This prevents external code from acquiring NestedJarFile's monitor while maintaining internal synchronization consistency. Add NestedJarFileLockOrderingDeadlockTests to deterministically reproduce the deadlock. The test uses a 5-second timeout for CI but supports diagnostic mode via -Dtest.deadlock.hang=true to capture thread dumps showing the deadlock. Includes documentation explaining why virtual thread deadlocks aren't auto-detected by HotSpot (virtual threads unmount when blocked on monitors). Fixes spring-projectsgh-51463 Fixes spring-projectsgh-51379 Signed-off-by: Ian Kettle <25729118+icikle@users.noreply.github.com>
…by exposed monitors in jar loading NestedJarFile and FileDataBlock both expose their monitors via synchronized(this), allowing external code (ClassLoader, reflection machinery) to acquire them directly. This creates AB-BA deadlock cycles: one thread holds an unrelated lock while waiting on the jar monitor, while another thread holds the jar monitor while waiting on the unrelated lock. NestedJarFile: - Remove synchronized blocks from read-only methods (hasEntry, getJarEntry, getComment) - Use atomic ensureOpen() validation that returns ZipContent reference - Keeps synchronization on methods that mutate state (getInputStream, size, close) - Maintains consistency with superclass JarFile synchronization contract FileDataBlock: - Replace simple synchronized blocks with atomic reference counting (AtomicInteger) - Implement double-checked locking for open() and close() state transitions - Minimize synchronized window to state mutations only - Eliminate exposed monitor for file channel lifecycle Both changes eliminate exposed monitors while maintaining thread-safety through atomic operations and minimal synchronization. Verified in production with 3 consecutive successful deployments. Fixes spring-projectsgh-51463 Fixes spring-projectsgh-51379 Signed-off-by: Ian Kettle <25729118+icikle@users.noreply.github.com>
…taBlock uncovered by NestedJarFile tests Reverted the previous attempt at a FileDataBlock change, and added concurrency tests (AI assisted) to prove the concurrency behaviour of the NestedJarFile fix under load. This uncovered a window in the existing FileDataBlock code between two separate synchronized blocks where execution could become inconsistent. The fix was to make referenceCount volatile so ensureOpen can accurately check state without synchronization, and to add a second referenceCount check inside read() itself, closing the window between checking and using the buffer. Signed-off-by: Ian Kettle <25729118+icikle@users.noreply.github.com>
206bf8e to
33947c9
Compare
| * support for slicing. | ||
| * | ||
| * @author Phillip Webb | ||
| * @author Ian Kettle |
There was a problem hiding this comment.
Not sure if i should add this - changes to the concurrency feel significant enough to meet threshold in the contribution doc. If its added here it should add to the NestedJarFile change too.
| if (pos < 0) { | ||
| throw new IllegalArgumentException("Position must not be negative"); | ||
| } | ||
| ensureOpen(ClosedChannelException::new); |
There was a problem hiding this comment.
Previously entered a sync block in the ensureOpen then the lines below here operated outside of sync and then the read goes back into synchronised with the assumption that the block hasn't been close between.
2 participants
I've been investigating this issue and believe I've identified the root cause: both NestedJarFile and FileDataBlock expose their monitors via synchronized(this), creating deadlock within ClassLoader internals during concurrent jar access under reflection/class-loading/resource loading scenarios.
Proposed fix: https://github.com/icikle/spring-boot/tree/fix/gh-51463-nested-jar-locking
Changes:
NestedJarFile: Remove synchronized blocks from read-only methods, use atomic ensureOpen() validation to eliminate the need for synchronization (not just removing synchronized blocks but making them unnecessary). Keeps synchronization where state is mutated, maintaining consistency with superclass JarFile contract.
FileDataBlock: Reduce scope of synchronized blocks by integrating atomic integer for reference counting (AtomicInteger) and double-checked locking for state transitions. Minimizes the synchronized window to state mutations only.
Both changes eliminate or reduce exposed monitors while maintaining thread-safety through atomic operations and minimal synchronization.
Production validation: 10 consecutive deployments (so far) with the fix deployed successfully so far - deadlocks eliminated, no hangs, no regressions observed.
Some further context:
This isn't a virtual thread issue as such but the thread dump from a locked system is different between virtual threads and real threads and appears to be easier to hit with virtual threads. The test case provided uses real threads by necessity as code base is JDK 17 language level.
The test case is pretty direct whereas the real world scenario is more complex. I want to try and bring this closer to our own thread dump as posted by @mikee on #51379 (@mikee is a colleague - we are looking at same issue together).
Post @mikee 's #51379 (comment) we upgraded to spring boot 4.1.1 to verify that the upgrade didn't fix.
Using real threads the test case shows a deadlock
Using virtual threads no deadlock is observed in the textual thread dump but the json thread dump shows the blocked virtual threads and what they are waiting on. This is consistent with the initial bug reports - no deadlock shown.
We have only observed the issue when we run with the PropertiesLauncher. When a project using our software needs to include its own jar files with either java or configuration files we need to use the properties launcher with the -Dloader.path option.
In development or environments where all resources are in the fat jar we use the JarLauncher and do not see this issue.
There is more detail in the test case javadocs.
Once the deadlock relating to the NestedJarFile was sorted with the NestedJarFile fix, the deployment to a container immediately hit the issue with FileDataBlock. It may be better to separate that from this PR but for my case the FileDataBlock fix is also needed.
I am also looking at how our application can be contributing to the issue but as we are not the only ones hitting it do believe a low level fix would be preferable. Also the fact that the issue only shows with the PropertiesLauncher points to that being at least contributing to the issue.
If this is isolated to the PropertiesLauncher then this bug would only effect a comparatively small subset of users. I don't have real numbers but the estimates I've seen are ~5% of users use PropertiesLauncher.
While both launchers use the NestedJarFile the PropertiesLauncher uses it in a more dynamic and less predictable way as it discovers jars at runtime rather than via Jar metadata.
The thread dumps from the test with virtual threads enabled are below and I believe are similar the issue as reported :
vthread-jcmd-threaddump.json
vthread-jcmd-threaddump.txt - Doesn't feature the NestedJarFile as the virtual threads are unmounted.