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
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.
| if (inflaterCache != null) { | ||
| 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.
|
Thanks for the new proposal, @icikle. I haven't had a chance to review this in detail, but one thing I have noticed is that this still changes the locking model of One example of this change is that calling I would really like to see a real-world reproduction of the hang so that we can interrogate the JVM and hopefully determine exactly why the problem's occurring. That should help us to reach an understanding of why only |
|
The method that has been seen as blocked in the thread dumps is the NestedJarFile.hasEntry(String name). This method isn't an override of a superclass method so the change to this method is not changing any behaviour of the superclass. The others I changed to be consistent but to mitigate risk and ensure consistency I could roll this back so that only change the hasEntry method in this class actually has the synchronized block removed. The change to ensureOpen still ensures that hasEntry is safe under concurrent access. Absolutely understand the desire to have full confidence in any change in this area. The change in NestedJarFileResources fixes a genuine bug validated with a test. If you would like me to move this to a separate PR please let me know. The change in FileDataBlock could also go separately but has more context along with the NestedJarFile change. I'll attempt to replicate in another way. Doing it in a unit test is too deterministic and as such appears invalid. Given I can use our application to replicate this I'll create a small SpringBoot project along with docker container and load test to attempt to replicate. I'll put this in a separate repository and share it. |
|
@wilkinsona I've made a start on the separate app for replicating the reported block on hasEntry. I'll also verify whether our application that is encountering the issue is fixed by just the hasEntry change and I'll take out the changes to other methods |
68ca62c to
97c689d
Compare
|
@wilkinsona - I've peeled it back so now it is only the hasEntry that has had the synchronization removed. As this is not an override from a superclass I'm hoping this may be acceptable as long as the concurrency is still guaranteed - which I think it is. There are a couple of changes here moving synchronized blocks to accomplish this. I believe these do not negatively effect the synchronization of the effected method which is getNestedJarEntry. Previously this called into getVersionedContentEntry and getContentEntry which had their own synchonized blocks. I've moved the synchronized block up to getNestedeJarEntry itself. This, along with including the this.lastEntry in the block, appears to be a better boundary. This change means that getContentEntry and getMetaInfVersionsInfo (both private methods) do not need to have their own synchronized blocks which enabled the hasEntry to complete without synchronization. I have put an app that reproduces the issue here - https://github.com/icikle/springboot-nestedjarfile-proof. It really just starts up and tries to read a bunch of files from the jar files with virtual threads. It blocks every time I run it. There are thread dumps in the repository. I need to add a readme in here but it should be understandable. It will build with spring boot 4.1.1 but if you build with "./gradlew build -PloaderVersion=4.1.2-SNAPSHOT" it will use a snapshot for the loader. With my changes as per this PR the application no longer blocks on hasEntry - the reported issue. However, as it is intentionally causing a lot of concurrent reads from the jars I am still getting threads blocked on FileDataBlock instead. I've looked at options for decreasing contention here but nothing I've come up with as yet is a small change. The options I've come up with are - enhance the FileDataBlock to support a pool of ByteBuffers allowing concurrent reads of different blocks (feels like overkill if concurrency isn't happening), using a read write lock - which isn't a great fit and would mean a big change, or use a stamped lock which is the lowest impact change. I investigated the stamped lock approach and will push that to a separate branch - I haven't yet determined if this is worth pursuing. The change I have in this PR - as it stands now - does fix the issue with NestedJarFile#hasEntry getting blocked for my application. It is now starting up correctly with just this change. At the end of the day, when an app like this sample app is intentionally causing contention (much higher than what my real application does), eliminating all blocking behaviour in an area like this is unrealistic. |
|
@wilkinsona I've also opened #51744 with just the change to fix the concurrency issue in NestedJarFileResources. Also there is now a brief README in https://github.com/icikle/springboot-nestedjarfile-proof. |
…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>
…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>
… synchronized blocks from hasEntry (where we are seeing the blockage), getMetaInfVersionsInfo and getContentEntry. Both getMetaInfVersionsInfo and getContent entry are called from hasEntry. This removes synchronization from the hasEntry path. The other path that calls these methods is getNestedJarEntry which has had a synchronized(this) added around these method calls to preserve the synchronized behaviour and reduce monitor entry/exit as now is only a single synchronized block rather than two. The new synchronized block also contains the creation and setting of the NestedJarEntry to the.lastEntry for additional confidence that this.lastEntry actually is the lastEntry. Signed-off-by: Ian Kettle <25729118+icikle@users.noreply.github.com>
97c689d to
64c3d03
Compare
|
51744 was merged so I've rebased this. Note that NestedJarFileConcurrencyTest in this was also in that PR and was removed as while valuable for proving the fix is not ideal for ongoing regression tests. |
Signed-off-by: Ian Kettle <25729118+icikle@users.noreply.github.com>
3 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.