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).
There was a problem hiding this comment.
Added additional concurrency tests to verify the safety of this change as it stands. These tests are what found the bug in NestedJarFileResources. The test (NestedJarFileConcurrencyTest) was intended to confirm that the reference count check in FileDataBlock is sufficient to make a stale read of the non-volatile zipContent field fail cleanly.
I have been unable to reproduce the scenario of a stale read, but the possible difference is that a ClosedChannelException is thrown where an IllegalStateException was previously thrown. I did consider catching it and rethrowing as IllegalStateException, but consider the scenario unlikely enough that I have not.
| } | ||
|
|
||
| <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.
There was a problem hiding this comment.
Reading my comment I thought it worth clarifying. Previously both the ensureOpen (called on line 73) and the read (called on line 84) synchronized internally. The block in between happens outside of the synchronisation and the read call had no check that the FileDataBlock was still open (or had a non-0 number of references). So there was scope for the reference count to change between the synchonized blocks and the read call would fail. in this changed version the ensureOpen doesn't contain a synchronized block but instead reads from the volatile field and will only throw an exception if the referenceCount is 0. The read method now checks the referenceCount as well and the exception supplier is now passed in for consistent exception behaviour.
…ace in NestedJarFileResources The concurrency tests added for the NestedJarFile changes uncovered a pre-existing bug, verified against the pre-existing code. A stream close racing a jar close could pass endOrCacheInflater()'s guard on a local cache reference and then find the field already cleared, throwing a NullPointerException from the cleanup action. Dereference the local reference, and clear the field while holding the cache monitor. Enhance the tests for additional confidence in the concurrency behaviour of the NestedJarFile and FileDataBlock changes. Signed-off-by: Ian Kettle <25729118+icikle@users.noreply.github.com>
| @@ -136,7 +136,7 @@ private void endOrCacheInflater(Inflater inflater) { | |||
| synchronized (inflaterCache) { | |||
| if (this.inflaterCache == inflaterCache && inflaterCache.size() < INFLATER_CACHE_LIMIT) { | |||
| inflater.reset(); | |||
There was a problem hiding this comment.
Since its synchronising on inflaterCache the field this.inflaterCache could have been nulled out between line 137 and 139. This prevents a NPE from happening by working on the local variable but real fix is moving the nulling of the this.inflaterCache to inside the synchronized block in the releaseInflators method. The NestedJarFileConcurrencyTest hit this:
NullPointerException: Cannot invoke "java.util.Deque.add(Object)" because "this.inflaterCache" is null at NestedJarFileResources.endOrCacheInflater(NestedJarFileResources.java:139)
I ran the test both with my version of NestedJarFile and the original version and the result was the same so this was a latent bug uncovered by hitting it hard with the concurrency test - not a regression caused by my changes.
| finally { | ||
| this.inflaterCache = null; | ||
| finally { | ||
| this.inflaterCache = null; |
There was a problem hiding this comment.
Moves the nulling of inflaterCache into the synchronized block - this would be the minimal change to make this class safe. We could just have this change and the other 2 changes in this class are not technically needed.
| * | ||
| * @author Ian Kettle | ||
| */ | ||
| class NestedJarFileLockOrderingDeadlockTests { |
There was a problem hiding this comment.
This is the test that attempts to replicate issue as reported. I think this may be the only of the 3 tests we want to retain as it is deterministic whereas the other 2 are concurrency tests and less suited for a CI environment.
| * | ||
| * @author Ian Kettle | ||
| */ | ||
| class NestedJarFileConcurrencyTests { |
There was a problem hiding this comment.
Possibly not a good candidate for retaining for CI due to the type of test.
| * | ||
| * @author Ian Kettle | ||
| */ | ||
| class FileDataBlockConcurrencyTests { |
There was a problem hiding this comment.
Possibly not a good candidate for retaining for CI due to the type of test.
2 participants
I've been investigating this issue and believe I've identified the root cause: both NestedJarFile exposes its monitors via synchronized(this), creating deadlock within ClassLoader internals during concurrent jar access under reflection/class-loading/resource loading scenarios. This was the main issue that directly contributes to the reported bug gh51463.
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: Identified that there was a gap between the synchronized blocks in the read method that could result in the second block being entered in an invalid state (fileAccess with 0 references). Added reference check to the FileAccess#read to ensure consistency. With this change the need for a synchonized block in ensureOpen was negated by changing referenceCount to be volatile so that ensureOpen can fail based on the current count without synchronisation. Synchronization could be maintained in the ensureOpen but its now unnecessary so I've removed it so that the FileDataBlock#read doesn't create need to enter synchroniztion twice.
NestedJarFileResources: Tests written to test concurrency of NestedJarFile change identified a gap in NestedJarFileResources that result in an inconsistent state. Running the test against the original versions of NestedJarFile and FileDataBlock confirmed that this was an existing issue which could be hit in current codebase. Fix to this is small.
These changes either reduce the amount of synchronisation or make it more precise. The reductions mean less contention for monitors and less possibility of deadlock; the NestedJarFileResources change instead brings a critical path into the synchronized block to fix a latent bug.
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.