From 48124cdbae26899a6f180e59dc2713a0e7615bf8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oddbj=C3=B8rn=20Kvalsund?= Date: Wed, 13 Mar 2024 17:56:06 +0100 Subject: [PATCH 1/5] Use ReentrantLock instead of synchronized to avoid deadlock on pinning --- .../jackson/databind/deser/DeserializerCache.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/fasterxml/jackson/databind/deser/DeserializerCache.java b/src/main/java/com/fasterxml/jackson/databind/deser/DeserializerCache.java index 032723c814..28127c1af0 100644 --- a/src/main/java/com/fasterxml/jackson/databind/deser/DeserializerCache.java +++ b/src/main/java/com/fasterxml/jackson/databind/deser/DeserializerCache.java @@ -1,6 +1,7 @@ package com.fasterxml.jackson.databind.deser; import java.util.HashMap; +import java.util.concurrent.locks.ReentrantLock; import com.fasterxml.jackson.annotation.JsonFormat; import com.fasterxml.jackson.databind.*; @@ -52,6 +53,12 @@ public final class DeserializerCache protected final HashMap> _incompleteDeserializers = new HashMap>(8); + + /** + * We hold an explicit lock while creating deserializers to avoid creating duplicates. + */ + private final ReentrantLock _incompleteDeserializersLock = new ReentrantLock(); + /* /********************************************************** /* Life-cycle @@ -246,7 +253,9 @@ protected JsonDeserializer _createAndCacheValueDeserializer(Deserializat * limitations necessary to ensure that only completely initialized ones * are visible and used. */ - synchronized (_incompleteDeserializers) { + try { + _incompleteDeserializersLock.lock(); + // Ok, then: could it be that due to a race condition, deserializer can now be found? JsonDeserializer deser = _findCachedDeserializer(type); if (deser != null) { @@ -269,6 +278,8 @@ protected JsonDeserializer _createAndCacheValueDeserializer(Deserializat _incompleteDeserializers.clear(); } } + } finally { + _incompleteDeserializersLock.unlock(); } } From 2668e5fee5abfc3be899ff485ff859870f795355 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oddbj=C3=B8rn=20Kvalsund?= Date: Fri, 15 Mar 2024 11:36:40 +0100 Subject: [PATCH 2/5] Replace most usage of synchronized with ReentrantLock --- .../databind/deser/BeanDeserializerBase.java | 13 ++++- .../databind/deser/std/DateDeserializers.java | 8 +++- .../databind/deser/std/EnumDeserializer.java | 8 +++- .../deser/std/StdKeyDeserializer.java | 13 ++++- .../jsontype/impl/TypeDeserializerBase.java | 8 +++- .../jackson/databind/ser/SerializerCache.java | 48 +++++++++++++++---- .../jackson/databind/type/TypeFactory.java | 4 ++ 7 files changed, 86 insertions(+), 16 deletions(-) diff --git a/src/main/java/com/fasterxml/jackson/databind/deser/BeanDeserializerBase.java b/src/main/java/com/fasterxml/jackson/databind/deser/BeanDeserializerBase.java index 3007714a1d..05c774f1b5 100644 --- a/src/main/java/com/fasterxml/jackson/databind/deser/BeanDeserializerBase.java +++ b/src/main/java/com/fasterxml/jackson/databind/deser/BeanDeserializerBase.java @@ -4,6 +4,7 @@ import java.lang.reflect.Constructor; import java.lang.reflect.InvocationTargetException; import java.util.*; +import java.util.concurrent.locks.ReentrantLock; import com.fasterxml.jackson.annotation.*; @@ -180,6 +181,8 @@ public abstract class BeanDeserializerBase */ protected transient HashMap> _subDeserializers; + private final ReentrantLock _subDeserializersLock = new ReentrantLock(); + /** * If one of properties has "unwrapped" value, we need separate * helper object @@ -1885,8 +1888,11 @@ protected JsonDeserializer _findSubclassDeserializer(DeserializationCont JsonDeserializer subDeser; // First: maybe we have already created sub-type deserializer? - synchronized (this) { + try { + _subDeserializersLock.lock(); subDeser = (_subDeserializers == null) ? null : _subDeserializers.get(new ClassKey(bean.getClass())); + } finally { + _subDeserializersLock.unlock(); } if (subDeser != null) { return subDeser; @@ -1902,11 +1908,14 @@ protected JsonDeserializer _findSubclassDeserializer(DeserializationCont subDeser = ctxt.findRootValueDeserializer(type); // Also, need to cache it if (subDeser != null) { - synchronized (this) { + try { + _subDeserializersLock.lock(); if (_subDeserializers == null) { _subDeserializers = new HashMap>();; } _subDeserializers.put(new ClassKey(bean.getClass()), subDeser); + } finally { + _subDeserializersLock.unlock(); } } return subDeser; diff --git a/src/main/java/com/fasterxml/jackson/databind/deser/std/DateDeserializers.java b/src/main/java/com/fasterxml/jackson/databind/deser/std/DateDeserializers.java index 4b96ca42d3..a634459276 100644 --- a/src/main/java/com/fasterxml/jackson/databind/deser/std/DateDeserializers.java +++ b/src/main/java/com/fasterxml/jackson/databind/deser/std/DateDeserializers.java @@ -4,6 +4,7 @@ import java.lang.reflect.Constructor; import java.text.*; import java.util.*; +import java.util.concurrent.locks.ReentrantLock; import com.fasterxml.jackson.annotation.JsonFormat; @@ -69,6 +70,8 @@ protected abstract static class DateBasedDeserializer */ protected final DateFormat _customFormat; + private final ReentrantLock _customFormatLock = new ReentrantLock(); + /** * Let's also keep format String for reference, to use for error messages */ @@ -188,13 +191,16 @@ protected java.util.Date _parseDate(JsonParser p, DeserializationContext ctxt) } return null; } - synchronized (_customFormat) { + try { + _customFormatLock.lock(); try { return _customFormat.parse(str); } catch (ParseException e) { return (java.util.Date) ctxt.handleWeirdStringValue(handledType(), str, "expected format \"%s\"", _formatString); } + } finally { + _customFormatLock.unlock(); } } } diff --git a/src/main/java/com/fasterxml/jackson/databind/deser/std/EnumDeserializer.java b/src/main/java/com/fasterxml/jackson/databind/deser/std/EnumDeserializer.java index 68c2be07c6..82fbe956b0 100644 --- a/src/main/java/com/fasterxml/jackson/databind/deser/std/EnumDeserializer.java +++ b/src/main/java/com/fasterxml/jackson/databind/deser/std/EnumDeserializer.java @@ -20,6 +20,7 @@ import com.fasterxml.jackson.databind.util.CompactStringObjectMap; import com.fasterxml.jackson.databind.util.EnumResolver; import java.util.Optional; +import java.util.concurrent.locks.ReentrantLock; /** * Deserializer class that can deserialize instances of @@ -54,6 +55,8 @@ public class EnumDeserializer */ protected volatile CompactStringObjectMap _lookupByToString; + private final ReentrantLock _lookupLock = new ReentrantLock(); + protected final Boolean _caseInsensitive; private Boolean _useDefaultValueForUnknownEnum; @@ -472,13 +475,16 @@ protected Class _enumClass() { protected CompactStringObjectMap _getToStringLookup(DeserializationContext ctxt) { CompactStringObjectMap lookup = _lookupByToString; if (lookup == null) { - synchronized (this) { + try { + _lookupLock.lock(); lookup = _lookupByToString; if (lookup == null) { lookup = EnumResolver.constructUsingToString(ctxt.getConfig(), _enumClass()) .constructLookup(); _lookupByToString = lookup; } + } finally { + _lookupLock.unlock(); } } return lookup; diff --git a/src/main/java/com/fasterxml/jackson/databind/deser/std/StdKeyDeserializer.java b/src/main/java/com/fasterxml/jackson/databind/deser/std/StdKeyDeserializer.java index d7eabc484a..d2713560c2 100644 --- a/src/main/java/com/fasterxml/jackson/databind/deser/std/StdKeyDeserializer.java +++ b/src/main/java/com/fasterxml/jackson/databind/deser/std/StdKeyDeserializer.java @@ -8,6 +8,7 @@ import java.net.URI; import java.net.URL; import java.util.*; +import java.util.concurrent.locks.ReentrantLock; import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.core.io.NumberInput; @@ -390,6 +391,8 @@ final static class EnumKD extends StdKeyDeserializer */ protected final EnumResolver _byEnumNamingResolver; + private final ReentrantLock _resolverLock = new ReentrantLock(); + protected final Enum _enumDefaultValue; protected EnumKD(EnumResolver er, AnnotatedMethod factory) { @@ -472,13 +475,16 @@ private EnumResolver _getToStringResolver(DeserializationContext ctxt) { EnumResolver res = _byToStringResolver; if (res == null) { - synchronized (this) { + try { + _resolverLock.lock(); res = _byToStringResolver; if (res == null) { res = EnumResolver.constructUsingToString(ctxt.getConfig(), _byNameResolver.getEnumClass()); _byToStringResolver = res; } + } finally { + _resolverLock.unlock(); } } return res; @@ -495,13 +501,16 @@ private EnumResolver _getToStringResolver(DeserializationContext ctxt) private EnumResolver _getIndexResolver(DeserializationContext ctxt) { EnumResolver res = _byIndexResolver; if (res == null) { - synchronized (this) { + try { + _resolverLock.lock(); res = _byIndexResolver; if (res == null) { res = EnumResolver.constructUsingIndex(ctxt.getConfig(), _byNameResolver.getEnumClass()); _byIndexResolver = res; } + } finally { + _resolverLock.unlock(); } } return res; diff --git a/src/main/java/com/fasterxml/jackson/databind/jsontype/impl/TypeDeserializerBase.java b/src/main/java/com/fasterxml/jackson/databind/jsontype/impl/TypeDeserializerBase.java index 2af739da33..5d24e51e25 100644 --- a/src/main/java/com/fasterxml/jackson/databind/jsontype/impl/TypeDeserializerBase.java +++ b/src/main/java/com/fasterxml/jackson/databind/jsontype/impl/TypeDeserializerBase.java @@ -3,6 +3,7 @@ import java.io.IOException; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.locks.ReentrantLock; import com.fasterxml.jackson.annotation.JsonTypeInfo; @@ -42,6 +43,8 @@ public abstract class TypeDeserializerBase */ protected final JavaType _defaultImpl; + protected final ReentrantLock _defaultImplLock = new ReentrantLock(); + /** * Name of type property used; needed for non-property versions too, * in cases where type id is to be exposed as part of JSON. @@ -223,12 +226,15 @@ protected final JsonDeserializer _findDefaultImplDeserializer(Deserializ return NullifyingDeserializer.instance; } - synchronized (_defaultImpl) { + try { + _defaultImplLock.lock(); if (_defaultImplDeserializer == null) { _defaultImplDeserializer = ctxt.findContextualValueDeserializer( _defaultImpl, _property); } return _defaultImplDeserializer; + } finally { + _defaultImplLock.unlock(); } } diff --git a/src/main/java/com/fasterxml/jackson/databind/ser/SerializerCache.java b/src/main/java/com/fasterxml/jackson/databind/ser/SerializerCache.java index c9b49f77eb..e2f625be4e 100644 --- a/src/main/java/com/fasterxml/jackson/databind/ser/SerializerCache.java +++ b/src/main/java/com/fasterxml/jackson/databind/ser/SerializerCache.java @@ -1,6 +1,7 @@ package com.fasterxml.jackson.databind.ser; import java.util.concurrent.atomic.AtomicReference; +import java.util.concurrent.locks.ReentrantLock; import com.fasterxml.jackson.databind.*; import com.fasterxml.jackson.databind.ser.impl.ReadOnlyClassToSerializerMap; @@ -45,6 +46,8 @@ public final class SerializerCache */ private final LookupCache> _sharedMap; + private final ReentrantLock _sharedMapLock = new ReentrantLock(); + /** * Most recent read-only instance, created from _sharedMap, if any. */ @@ -107,29 +110,41 @@ public synchronized int size() { */ public JsonSerializer untypedValueSerializer(Class type) { - synchronized (this) { + try { + _sharedMapLock.lock(); return _sharedMap.get(new TypeKey(type, false)); + } finally { + _sharedMapLock.unlock(); } } public JsonSerializer untypedValueSerializer(JavaType type) { - synchronized (this) { + try { + _sharedMapLock.lock(); return _sharedMap.get(new TypeKey(type, false)); + } finally { + _sharedMapLock.unlock(); } } public JsonSerializer typedValueSerializer(JavaType type) { - synchronized (this) { + try { + _sharedMapLock.lock(); return _sharedMap.get(new TypeKey(type, true)); + } finally { + _sharedMapLock.unlock(); } } public JsonSerializer typedValueSerializer(Class cls) { - synchronized (this) { + try { + _sharedMapLock.lock(); return _sharedMap.get(new TypeKey(cls, true)); + } finally { + _sharedMapLock.unlock(); } } @@ -146,21 +161,27 @@ public JsonSerializer typedValueSerializer(Class cls) */ public void addTypedSerializer(JavaType type, JsonSerializer ser) { - synchronized (this) { + try { + _sharedMapLock.lock(); if (_sharedMap.put(new TypeKey(type, true), ser) == null) { // let's invalidate the read-only copy, too, to get it updated _readOnlyMap.set(null); } + } finally { + _sharedMapLock.unlock(); } } public void addTypedSerializer(Class cls, JsonSerializer ser) { - synchronized (this) { + try { + _sharedMapLock.lock(); if (_sharedMap.put(new TypeKey(cls, true), ser) == null) { // let's invalidate the read-only copy, too, to get it updated _readOnlyMap.set(null); } + } finally { + _sharedMapLock.unlock(); } } @@ -168,7 +189,8 @@ public void addAndResolveNonTypedSerializer(Class type, JsonSerializer type, JsonSerializer rawType, JavaType fullType, SerializerProvider provider) throws JsonMappingException { - synchronized (this) { + try { + _sharedMapLock.lock(); Object ob1 = _sharedMap.put(new TypeKey(rawType, false), ser); Object ob2 = _sharedMap.put(new TypeKey(fullType, false), ser); if ((ob1 == null) || (ob2 == null)) { @@ -222,6 +250,8 @@ public void addAndResolveNonTypedSerializer(Class rawType, JavaType fullType, if (ser instanceof ResolvableSerializer) { ((ResolvableSerializer) ser).resolve(provider); } + } finally { + _sharedMapLock.unlock(); } } diff --git a/src/main/java/com/fasterxml/jackson/databind/type/TypeFactory.java b/src/main/java/com/fasterxml/jackson/databind/type/TypeFactory.java index ce35d06ff0..233c871180 100644 --- a/src/main/java/com/fasterxml/jackson/databind/type/TypeFactory.java +++ b/src/main/java/com/fasterxml/jackson/databind/type/TypeFactory.java @@ -1758,6 +1758,10 @@ protected JavaType _fromVariable(ClassStack context, TypeVariable var, TypeBi // // No good way to reproduce but since this should not be on critical path, let's add // syncing as it seems potentially necessary. + // + // 15-Mar-2024, oddbjornkvalsund: Not replacing this usage of 'synchronized' with an explicit lock as elsewhere. + // Assuming java.lang.reflect.TypeVariable.getBounds() does not do IO or other calls causing the thread to + // park and potentially pin to the platform thread. synchronized (var) { bounds = var.getBounds(); } From 588b21ef35f04036a42a38f7ecf8a9cdd2def2de Mon Sep 17 00:00:00 2001 From: PJ Fanning Date: Thu, 28 Mar 2024 18:42:07 +0100 Subject: [PATCH 3/5] Update main.yml so that ci runs for 2.18 branch (#4457) --- .github/workflows/main.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 97c6960cf0..74960e6ba1 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -4,6 +4,7 @@ on: branches: - master - "3.0" + - "2.18" - "2.17" paths-ignore: - "README.md" @@ -12,6 +13,7 @@ on: branches: - master - "3.0" + - "2.18" - "2.17" paths-ignore: - "README.md" From d51268309a6d6688e82adecf31387ec24d04f7e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oddbj=C3=B8rn=20Kvalsund?= Date: Fri, 29 Mar 2024 11:03:49 +0100 Subject: [PATCH 4/5] Revert "Replace most usage of synchronized with ReentrantLock" This reverts commit 2668e5fee5abfc3be899ff485ff859870f795355. --- .../databind/deser/BeanDeserializerBase.java | 13 +---- .../databind/deser/std/DateDeserializers.java | 8 +--- .../databind/deser/std/EnumDeserializer.java | 8 +--- .../deser/std/StdKeyDeserializer.java | 13 +---- .../jsontype/impl/TypeDeserializerBase.java | 8 +--- .../jackson/databind/ser/SerializerCache.java | 48 ++++--------------- .../jackson/databind/type/TypeFactory.java | 4 -- 7 files changed, 16 insertions(+), 86 deletions(-) diff --git a/src/main/java/com/fasterxml/jackson/databind/deser/BeanDeserializerBase.java b/src/main/java/com/fasterxml/jackson/databind/deser/BeanDeserializerBase.java index 05c774f1b5..3007714a1d 100644 --- a/src/main/java/com/fasterxml/jackson/databind/deser/BeanDeserializerBase.java +++ b/src/main/java/com/fasterxml/jackson/databind/deser/BeanDeserializerBase.java @@ -4,7 +4,6 @@ import java.lang.reflect.Constructor; import java.lang.reflect.InvocationTargetException; import java.util.*; -import java.util.concurrent.locks.ReentrantLock; import com.fasterxml.jackson.annotation.*; @@ -181,8 +180,6 @@ public abstract class BeanDeserializerBase */ protected transient HashMap> _subDeserializers; - private final ReentrantLock _subDeserializersLock = new ReentrantLock(); - /** * If one of properties has "unwrapped" value, we need separate * helper object @@ -1888,11 +1885,8 @@ protected JsonDeserializer _findSubclassDeserializer(DeserializationCont JsonDeserializer subDeser; // First: maybe we have already created sub-type deserializer? - try { - _subDeserializersLock.lock(); + synchronized (this) { subDeser = (_subDeserializers == null) ? null : _subDeserializers.get(new ClassKey(bean.getClass())); - } finally { - _subDeserializersLock.unlock(); } if (subDeser != null) { return subDeser; @@ -1908,14 +1902,11 @@ protected JsonDeserializer _findSubclassDeserializer(DeserializationCont subDeser = ctxt.findRootValueDeserializer(type); // Also, need to cache it if (subDeser != null) { - try { - _subDeserializersLock.lock(); + synchronized (this) { if (_subDeserializers == null) { _subDeserializers = new HashMap>();; } _subDeserializers.put(new ClassKey(bean.getClass()), subDeser); - } finally { - _subDeserializersLock.unlock(); } } return subDeser; diff --git a/src/main/java/com/fasterxml/jackson/databind/deser/std/DateDeserializers.java b/src/main/java/com/fasterxml/jackson/databind/deser/std/DateDeserializers.java index a634459276..4b96ca42d3 100644 --- a/src/main/java/com/fasterxml/jackson/databind/deser/std/DateDeserializers.java +++ b/src/main/java/com/fasterxml/jackson/databind/deser/std/DateDeserializers.java @@ -4,7 +4,6 @@ import java.lang.reflect.Constructor; import java.text.*; import java.util.*; -import java.util.concurrent.locks.ReentrantLock; import com.fasterxml.jackson.annotation.JsonFormat; @@ -70,8 +69,6 @@ protected abstract static class DateBasedDeserializer */ protected final DateFormat _customFormat; - private final ReentrantLock _customFormatLock = new ReentrantLock(); - /** * Let's also keep format String for reference, to use for error messages */ @@ -191,16 +188,13 @@ protected java.util.Date _parseDate(JsonParser p, DeserializationContext ctxt) } return null; } - try { - _customFormatLock.lock(); + synchronized (_customFormat) { try { return _customFormat.parse(str); } catch (ParseException e) { return (java.util.Date) ctxt.handleWeirdStringValue(handledType(), str, "expected format \"%s\"", _formatString); } - } finally { - _customFormatLock.unlock(); } } } diff --git a/src/main/java/com/fasterxml/jackson/databind/deser/std/EnumDeserializer.java b/src/main/java/com/fasterxml/jackson/databind/deser/std/EnumDeserializer.java index 82fbe956b0..68c2be07c6 100644 --- a/src/main/java/com/fasterxml/jackson/databind/deser/std/EnumDeserializer.java +++ b/src/main/java/com/fasterxml/jackson/databind/deser/std/EnumDeserializer.java @@ -20,7 +20,6 @@ import com.fasterxml.jackson.databind.util.CompactStringObjectMap; import com.fasterxml.jackson.databind.util.EnumResolver; import java.util.Optional; -import java.util.concurrent.locks.ReentrantLock; /** * Deserializer class that can deserialize instances of @@ -55,8 +54,6 @@ public class EnumDeserializer */ protected volatile CompactStringObjectMap _lookupByToString; - private final ReentrantLock _lookupLock = new ReentrantLock(); - protected final Boolean _caseInsensitive; private Boolean _useDefaultValueForUnknownEnum; @@ -475,16 +472,13 @@ protected Class _enumClass() { protected CompactStringObjectMap _getToStringLookup(DeserializationContext ctxt) { CompactStringObjectMap lookup = _lookupByToString; if (lookup == null) { - try { - _lookupLock.lock(); + synchronized (this) { lookup = _lookupByToString; if (lookup == null) { lookup = EnumResolver.constructUsingToString(ctxt.getConfig(), _enumClass()) .constructLookup(); _lookupByToString = lookup; } - } finally { - _lookupLock.unlock(); } } return lookup; diff --git a/src/main/java/com/fasterxml/jackson/databind/deser/std/StdKeyDeserializer.java b/src/main/java/com/fasterxml/jackson/databind/deser/std/StdKeyDeserializer.java index d2713560c2..d7eabc484a 100644 --- a/src/main/java/com/fasterxml/jackson/databind/deser/std/StdKeyDeserializer.java +++ b/src/main/java/com/fasterxml/jackson/databind/deser/std/StdKeyDeserializer.java @@ -8,7 +8,6 @@ import java.net.URI; import java.net.URL; import java.util.*; -import java.util.concurrent.locks.ReentrantLock; import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.core.io.NumberInput; @@ -391,8 +390,6 @@ final static class EnumKD extends StdKeyDeserializer */ protected final EnumResolver _byEnumNamingResolver; - private final ReentrantLock _resolverLock = new ReentrantLock(); - protected final Enum _enumDefaultValue; protected EnumKD(EnumResolver er, AnnotatedMethod factory) { @@ -475,16 +472,13 @@ private EnumResolver _getToStringResolver(DeserializationContext ctxt) { EnumResolver res = _byToStringResolver; if (res == null) { - try { - _resolverLock.lock(); + synchronized (this) { res = _byToStringResolver; if (res == null) { res = EnumResolver.constructUsingToString(ctxt.getConfig(), _byNameResolver.getEnumClass()); _byToStringResolver = res; } - } finally { - _resolverLock.unlock(); } } return res; @@ -501,16 +495,13 @@ private EnumResolver _getToStringResolver(DeserializationContext ctxt) private EnumResolver _getIndexResolver(DeserializationContext ctxt) { EnumResolver res = _byIndexResolver; if (res == null) { - try { - _resolverLock.lock(); + synchronized (this) { res = _byIndexResolver; if (res == null) { res = EnumResolver.constructUsingIndex(ctxt.getConfig(), _byNameResolver.getEnumClass()); _byIndexResolver = res; } - } finally { - _resolverLock.unlock(); } } return res; diff --git a/src/main/java/com/fasterxml/jackson/databind/jsontype/impl/TypeDeserializerBase.java b/src/main/java/com/fasterxml/jackson/databind/jsontype/impl/TypeDeserializerBase.java index 5d24e51e25..2af739da33 100644 --- a/src/main/java/com/fasterxml/jackson/databind/jsontype/impl/TypeDeserializerBase.java +++ b/src/main/java/com/fasterxml/jackson/databind/jsontype/impl/TypeDeserializerBase.java @@ -3,7 +3,6 @@ import java.io.IOException; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.locks.ReentrantLock; import com.fasterxml.jackson.annotation.JsonTypeInfo; @@ -43,8 +42,6 @@ public abstract class TypeDeserializerBase */ protected final JavaType _defaultImpl; - protected final ReentrantLock _defaultImplLock = new ReentrantLock(); - /** * Name of type property used; needed for non-property versions too, * in cases where type id is to be exposed as part of JSON. @@ -226,15 +223,12 @@ protected final JsonDeserializer _findDefaultImplDeserializer(Deserializ return NullifyingDeserializer.instance; } - try { - _defaultImplLock.lock(); + synchronized (_defaultImpl) { if (_defaultImplDeserializer == null) { _defaultImplDeserializer = ctxt.findContextualValueDeserializer( _defaultImpl, _property); } return _defaultImplDeserializer; - } finally { - _defaultImplLock.unlock(); } } diff --git a/src/main/java/com/fasterxml/jackson/databind/ser/SerializerCache.java b/src/main/java/com/fasterxml/jackson/databind/ser/SerializerCache.java index e2f625be4e..c9b49f77eb 100644 --- a/src/main/java/com/fasterxml/jackson/databind/ser/SerializerCache.java +++ b/src/main/java/com/fasterxml/jackson/databind/ser/SerializerCache.java @@ -1,7 +1,6 @@ package com.fasterxml.jackson.databind.ser; import java.util.concurrent.atomic.AtomicReference; -import java.util.concurrent.locks.ReentrantLock; import com.fasterxml.jackson.databind.*; import com.fasterxml.jackson.databind.ser.impl.ReadOnlyClassToSerializerMap; @@ -46,8 +45,6 @@ public final class SerializerCache */ private final LookupCache> _sharedMap; - private final ReentrantLock _sharedMapLock = new ReentrantLock(); - /** * Most recent read-only instance, created from _sharedMap, if any. */ @@ -110,41 +107,29 @@ public synchronized int size() { */ public JsonSerializer untypedValueSerializer(Class type) { - try { - _sharedMapLock.lock(); + synchronized (this) { return _sharedMap.get(new TypeKey(type, false)); - } finally { - _sharedMapLock.unlock(); } } public JsonSerializer untypedValueSerializer(JavaType type) { - try { - _sharedMapLock.lock(); + synchronized (this) { return _sharedMap.get(new TypeKey(type, false)); - } finally { - _sharedMapLock.unlock(); } } public JsonSerializer typedValueSerializer(JavaType type) { - try { - _sharedMapLock.lock(); + synchronized (this) { return _sharedMap.get(new TypeKey(type, true)); - } finally { - _sharedMapLock.unlock(); } } public JsonSerializer typedValueSerializer(Class cls) { - try { - _sharedMapLock.lock(); + synchronized (this) { return _sharedMap.get(new TypeKey(cls, true)); - } finally { - _sharedMapLock.unlock(); } } @@ -161,27 +146,21 @@ public JsonSerializer typedValueSerializer(Class cls) */ public void addTypedSerializer(JavaType type, JsonSerializer ser) { - try { - _sharedMapLock.lock(); + synchronized (this) { if (_sharedMap.put(new TypeKey(type, true), ser) == null) { // let's invalidate the read-only copy, too, to get it updated _readOnlyMap.set(null); } - } finally { - _sharedMapLock.unlock(); } } public void addTypedSerializer(Class cls, JsonSerializer ser) { - try { - _sharedMapLock.lock(); + synchronized (this) { if (_sharedMap.put(new TypeKey(cls, true), ser) == null) { // let's invalidate the read-only copy, too, to get it updated _readOnlyMap.set(null); } - } finally { - _sharedMapLock.unlock(); } } @@ -189,8 +168,7 @@ public void addAndResolveNonTypedSerializer(Class type, JsonSerializer type, JsonSerializer rawType, JavaType fullType, SerializerProvider provider) throws JsonMappingException { - try { - _sharedMapLock.lock(); + synchronized (this) { Object ob1 = _sharedMap.put(new TypeKey(rawType, false), ser); Object ob2 = _sharedMap.put(new TypeKey(fullType, false), ser); if ((ob1 == null) || (ob2 == null)) { @@ -250,8 +222,6 @@ public void addAndResolveNonTypedSerializer(Class rawType, JavaType fullType, if (ser instanceof ResolvableSerializer) { ((ResolvableSerializer) ser).resolve(provider); } - } finally { - _sharedMapLock.unlock(); } } diff --git a/src/main/java/com/fasterxml/jackson/databind/type/TypeFactory.java b/src/main/java/com/fasterxml/jackson/databind/type/TypeFactory.java index 233c871180..ce35d06ff0 100644 --- a/src/main/java/com/fasterxml/jackson/databind/type/TypeFactory.java +++ b/src/main/java/com/fasterxml/jackson/databind/type/TypeFactory.java @@ -1758,10 +1758,6 @@ protected JavaType _fromVariable(ClassStack context, TypeVariable var, TypeBi // // No good way to reproduce but since this should not be on critical path, let's add // syncing as it seems potentially necessary. - // - // 15-Mar-2024, oddbjornkvalsund: Not replacing this usage of 'synchronized' with an explicit lock as elsewhere. - // Assuming java.lang.reflect.TypeVariable.getBounds() does not do IO or other calls causing the thread to - // park and potentially pin to the platform thread. synchronized (var) { bounds = var.getBounds(); } From 9d31ec7b804e47979cf3d5fc62a5b5c543708a49 Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Fri, 29 Mar 2024 12:41:37 -0700 Subject: [PATCH 5/5] Update release notes --- release-notes/CREDITS-2.x | 6 ++++++ release-notes/VERSION-2.x | 3 +++ 2 files changed, 9 insertions(+) diff --git a/release-notes/CREDITS-2.x b/release-notes/CREDITS-2.x index 3371e8dcfe..48339f1e4d 100644 --- a/release-notes/CREDITS-2.x +++ b/release-notes/CREDITS-2.x @@ -1765,3 +1765,9 @@ Kyrylo Merzlikin (kirmerzlikin@github) Miguel Mendes Ruiz (migmruiz@github) * Reported #4428: `ByteBuddy` scope went beyond `test` in version 2.17.0 (2.17.1) + +Oddbjørn Kvalsund (oddbjornkvalsund@github) + * Reported, contributed fix for #4430: Use `ReentrantLock` instead of `synchronized` + in `DeserializerCache` to avoid deadlock on pinning + (2.17.1) + diff --git a/release-notes/VERSION-2.x b/release-notes/VERSION-2.x index 6c56cd42e1..e7ea07bad7 100644 --- a/release-notes/VERSION-2.x +++ b/release-notes/VERSION-2.x @@ -13,6 +13,9 @@ Project: jackson-databind #4428: `ByteBuddy` scope went beyond `test` in version 2.17.0 (reported by Miguel M-R) (fix by Joo-Hyuk K) +#4430: Use `ReentrantLock` instead of `synchronized` in `DeserializerCache` + to avoid deadlock on pinning + (reported, fix contributed by Oddbjørn K) #4435: Cannot deserialize value of type `java.math.BigDecimal` from String ".05": not a valid representation (reported by @EAlf91)