Use ReentrantLock instead of synchronized in DeserializerCache to avoid deadlock on pinning
#4430
Conversation
|
The detailed thread stacktrace was produced by The following unit test illustrates the underlying problem. If you set both boolean flag to |
|
The InfoWorld article makes this sound like a Java bug that Oracle are seeking to fix. While Jackson will probably replace synchronized blocks with explicit locks, this is probably not going to happen in a patch release. |
|
@cowtowncoder From my testing, Reentrant locks are slower than synchronized blocks. This change could affect performance for the majority of users. One option is to make the behaviour configurable. One code path uses synchronized blocks and the other ReentrantLocks. |
|
@pjfanning I agree that the "proper" fix would be to fix the JVMs problematic behavior observed when synchronized threads park (more specifically "threads holding an object monitor") and this indeed being worked on by the JVM team, but I'm a bit worried that this will take months (if not years) to complete. For info there is currently 18 usages of
|
|
@oddbjornkvalsund You are free to fork jackson-databind and make whatever changes suit you. Unfortunately, I need to worry about all other jackson-databind users. I don't feel like hacking away at jackson-databind because of JDK bugs is something that should be rushed into. |
|
@pjfanning Sure, I have already forked and submitted this change for the one specific part of the code that caused problems for me. I'll be happy to dig into the remaining, hang on... |
|
@pjfanning There, I've now changed the remaining usages of I realize that there is quite possibly a ton of As to the more fundamental question of
...I have no strong opinions, but I do feel that if you choose to wait then you should give a big warning to users that Jackson cannot be used with JDK 21 or newer until this issue is fixed. The deadlock issue I observed is very real and hits very hard. |
is discouraged on virtual threads* |
|
Not every Java 21/22 user is using virtual threads. Let's also be straight - this a major bug in Java. It is not a Jackson bug. @oddbjornkvalsund if you want to cast stones - cast them in the right direction. |
There was a problem hiding this comment.
-1 from me - but I guess it's up to @cowtowncoder
My preference is make the locking configurable so that users can choose to use synchronized blocks or ReentrantLocks. If we can agree on where to put the configuration (maybe JsonFactory) and what to call it, I can do a POC that uses the config and refactor the DeserializerCache to support both lock types. If the POC looks good, we can roll it out elsewhere in jackson-databind and beyond that into other Jackson modules.
|
@mariofusco Making you aware of this issue since you are the biggest driver of the existing Jackson changes to avoid ThreadLocal caching. |
| @@ -69,6 +70,8 @@ protected abstract static class DateBasedDeserializer<T> | |||
| */ | |||
| protected final DateFormat _customFormat; | |||
|
|
|||
| private final ReentrantLock _customFormatLock = new ReentrantLock(); | |||
There was a problem hiding this comment.
This code is not 100% correct - If you have 2 deserializer instances with the same _customFormat instance then synchronizing it will prevent concurrent use of that _customFormat instance - your lock would not work properly in that case.
It is sort of annoying that Java never made DateFormat thread-safe. We could look into finding thread-safe alternatives - or adding a thread-safe class of our own that wraps DataFormat but that uses a locking mechanism to control access.
There was a problem hiding this comment.
I haven't researched this but I suspect we could find some java.time API to do date/time formatting that is thread safe - and therefore not need locking here at all.
There was a problem hiding this comment.
I agree, the existing DateFormat should be replaced with a java.time.format.DateTimeFormatter which is immutable and thread-safe and at that point this synchronization could be of course totally removed. However I guess that this could be addressed with a different pull request, immediately after this will be merged.
There was a problem hiding this comment.
I am not sure replacement would work, unfortunately, since we are talking about support for java.util.Date / java.util.Calendar; and if I remember correctly, format Strings used with SimpleDateFormat are different from those used with Java 8 date/time (and Joda).
So the better fix for users is to upgrade from java.util types to java.time.
So while I agree with all the ugliness of java.util world, I am not sure spending time on better support there is worth the effort.
There was a problem hiding this comment.
this Date code is not on critical path - users can avoid it using java.time classes instead of java.util Data/Calendar
so this could be left alone - and maybe tackled later
| @@ -42,6 +43,8 @@ public abstract class TypeDeserializerBase | |||
| */ | |||
| protected final JavaType _defaultImpl; | |||
|
|
|||
| protected final ReentrantLock _defaultImplLock = new ReentrantLock(); | |||
There was a problem hiding this comment.
Same problem as I described for DateDeserializers - what if JavaType _defaultImpl instance is used by more than 1 TypeDeserializerBase instance. This set of refactors seem really dangerous to me. I hate the idea of hacking like this.
There was a problem hiding this comment.
I agree that this is wrong. In my opinion the ReentrantLock should be moved inside the JavaType.
There was a problem hiding this comment.
I don't think JavaType would be the place: locking here is specifically to try to avoid duplicate lookups for deserializer used for "default type" (of given polymorphic base type).
However. Since this is not done for correctness but as (possibly misguided?) performance optimization, maybe even use of basic AtomicReference would work (accepting possible race condition).
|
@pjfanning I have not intended to cast any stones or blame anyone and I'm sorry if I came across as blunt or pointing fingers. I am pragmatic about this and only focused on making things work. 🙏 The complexities with Thanks for identifying the bugs in my code! I will address those if there proves to be any interest in merging this PR. 👍 |
With the removal of biased locking I believe that this is no longer so true in modern JVMs. Just to give you one more datapoint we replaced all |
|
@cowtowncoder @pjfanning @oddbjornkvalsund I gave a second look at this pull request and in particular tried to replace the legacy Regarding the problem at hand I believe that it is not necessary to replace all the synchronized blocks (and in particular it is not needed for the block parsing the date) but only for the blocks that perform an I/O operation inside them. In fact pinning is a problem only when the virtual thread performs an I/O operation and then it should be unmounted from its carrier but it can't since it is pinned. In essence my suggestion is:
|
|
@mariofusco That sounds like a good approach. Feel free to close this PR! For me, avoiding setting the "jdk.tracePinnedThreads" system property made the problem go away for now, but since this is the approach suggested by the JDK team for identifying pinning issues, it's not great that I can't use it. |
Used in that way that system property doesn't tell the whole story. As I tried to explain a pinned virtual thread is not necessarily a problem: |
| @@ -107,29 +110,41 @@ public synchronized int size() { | |||
| */ | |||
| public JsonSerializer<Object> untypedValueSerializer(Class<?> type) | |||
| { | |||
| synchronized (this) { | |||
| try { | |||
There was a problem hiding this comment.
It seems this should never cause issues? There's no I/O operations within Map (LRUMap), TypeKey is stateless.
In fact... I don't think synchronization should be needed at all here: LRUMap states:
Since Jackson 2.14, the implementation
...
Is thread-safe and does NOT require external synchronization
so could it be this is legacy, pre-2.14 locking, that was just left for no good reason...
There was a problem hiding this comment.
After reading through the code, this may not be as simple as I thought (see #4442): I think synchronized actually has secondary purpose to act as monitor for resolution of cyclic dependencies for POJO serializers (BeanSerializer). And if so it cannot really be safely removed (but is also a big potential performance concern at same time).
|
Ok, with a quick (but I hope not too quick) look, I agree that a minimal patch (PR) would be preferable -- just tackling observed case of
|
|
Could we go back to having this PR being about the one lock in DeserializerCache ? This locking code needs due respect and every change needs to be analysed on its own merits. We will look at each case - we just don't need to try to handle every case in 1 PR. |
|
@pjfanning Right, that's what I tried to say (but with too many words) :) |
|
@pjfanning Could you come up with minimal PR for |
|
@oddbjornkvalsund could you change this PR to just keep the DeserializerCache change? We can deal with the other classes separately. |
|
I created a speculative PR - #4456 |
This reverts commit 2668e5f.
Done! |
cowtowncoder
changed the title
ReentrantLock instead of synchronized in DeserializerCache to avoid deadlock on pinning
| /** | ||
| * We hold an explicit lock while creating deserializers to avoid creating duplicates. | ||
| */ | ||
| private final ReentrantLock _incompleteDeserializersLock = new ReentrantLock(); |
There was a problem hiding this comment.
@cowtowncoder can we keep this private as it is in the PR? I want to do follow up PRs and would like to not have to keep this name. I think it too specific. I also think that protected code is just as bad as public code in forcing us to maintain compatibility across versions. Private code can be refactored without worrying about users who write their own subclasses.
There was a problem hiding this comment.
Yes, it's fine to be private; no benefit/need for protected/package access here.
DeserializerCacheusessynchronizedto protect the_incompleteDeserializersmap from concurrent modification. This deadlocks when using virtual threads + IO within the synchronized block and you end up with a non-responsive VM and stacktraces looking like the one below. Note in particular the threads that have- waiting to lock <0x00000006104ab5e8> (a java.util.HashMap).Replacing the
synchronizedkeyword with aReentrantLockfixes the problem. See relevant info here: https://www.infoworld.com/article/3713220/java-virtual-threads-hit-with-pinning-issue.html and here: https://inside.java/2024/02/21/quality-heads-up/.