Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use ReentrantLock instead of synchronized in DeserializerCache to avoid deadlock on pinning #4430

Merged
merged 4 commits into from
Mar 29, 2024
Merged

Use ReentrantLock instead of synchronized in DeserializerCache to avoid deadlock on pinning #4430

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
48124cd
Use ReentrantLock instead of synchronized to avoid deadlock on pinning
oddbjornkvalsund Mar 13, 2024
2668e5f
Replace most usage of synchronized with ReentrantLock
oddbjornkvalsund Mar 15, 2024
43a2e8a
Merge branch '2.17' into 2.17
cowtowncoder Mar 28, 2024
d512683
Revert "Replace most usage of synchronized with ReentrantLock"
oddbjornkvalsund Mar 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/main/java/com/fasterxml/jackson/databind/deser/BeanDeserializerBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.*;

Expand Down Expand Up @@ -180,6 +181,8 @@ public abstract class BeanDeserializerBase
*/
protected transient HashMap<ClassKey, JsonDeserializer<Object>> _subDeserializers;

private final ReentrantLock _subDeserializersLock = new ReentrantLock();

/**
* If one of properties has "unwrapped" value, we need separate
* helper object
Expand Down Expand Up @@ -1885,8 +1888,11 @@ protected JsonDeserializer<Object> _findSubclassDeserializer(DeserializationCont
JsonDeserializer<Object> 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;
Expand All @@ -1902,11 +1908,14 @@ protected JsonDeserializer<Object> _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<ClassKey,JsonDeserializer<Object>>();;
}
_subDeserializers.put(new ClassKey(bean.getClass()), subDeser);
} finally {
_subDeserializersLock.unlock();
}
}
return subDeser;
Expand Down
13 changes: 12 additions & 1 deletion src/main/java/com/fasterxml/jackson/databind/deser/DeserializerCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.*;
Expand Down Expand Up @@ -52,6 +53,12 @@ public final class DeserializerCache
protected final HashMap<JavaType, JsonDeserializer<Object>> _incompleteDeserializers
= new HashMap<JavaType, JsonDeserializer<Object>>(8);


/**
* We hold an explicit lock while creating deserializers to avoid creating duplicates.
*/
private final ReentrantLock _incompleteDeserializersLock = new ReentrantLock();
cowtowncoder marked this conversation as resolved.
Show resolved Hide resolved

/*
/**********************************************************
/* Life-cycle
Expand Down Expand Up @@ -246,7 +253,9 @@ protected JsonDeserializer<Object> _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<Object> deser = _findCachedDeserializer(type);
if (deser != null) {
Expand All @@ -269,6 +278,8 @@ protected JsonDeserializer<Object> _createAndCacheValueDeserializer(Deserializat
_incompleteDeserializers.clear();
}
}
} finally {
_incompleteDeserializersLock.unlock();
}
}

Expand Down
8 changes: 7 additions & 1 deletion src/main/java/com/fasterxml/jackson/databind/deser/std/DateDeserializers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -69,6 +70,8 @@ protected abstract static class DateBasedDeserializer<T>
*/
protected final DateFormat _customFormat;

private final ReentrantLock _customFormatLock = new ReentrantLock();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* Let's also keep format String for reference, to use for error messages
*/
Expand Down Expand Up @@ -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();
}
}
}
Expand Down
8 changes: 7 additions & 1 deletion src/main/java/com/fasterxml/jackson/databind/deser/std/EnumDeserializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -54,6 +55,8 @@ public class EnumDeserializer
*/
protected volatile CompactStringObjectMap _lookupByToString;

private final ReentrantLock _lookupLock = new ReentrantLock();

protected final Boolean _caseInsensitive;

private Boolean _useDefaultValueForUnknownEnum;
Expand Down Expand Up @@ -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;
Expand Down
13 changes: 11 additions & 2 deletions src/main/java/com/fasterxml/jackson/databind/deser/std/StdKeyDeserializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down
8 changes: 7 additions & 1 deletion src/main/java/com/fasterxml/jackson/databind/jsontype/impl/TypeDeserializerBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -42,6 +43,8 @@ public abstract class TypeDeserializerBase
*/
protected final JavaType _defaultImpl;

protected final ReentrantLock _defaultImplLock = new ReentrantLock();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this is wrong. In my opinion the ReentrantLock should be moved inside the JavaType.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


/**
* Name of type property used; needed for non-property versions too,
* in cases where type id is to be exposed as part of JSON.
Expand Down Expand Up @@ -223,12 +226,15 @@ protected final JsonDeserializer<Object> _findDefaultImplDeserializer(Deserializ
return NullifyingDeserializer.instance;
}

synchronized (_defaultImpl) {
try {
_defaultImplLock.lock();
if (_defaultImplDeserializer == null) {
_defaultImplDeserializer = ctxt.findContextualValueDeserializer(
_defaultImpl, _property);
}
return _defaultImplDeserializer;
} finally {
_defaultImplLock.unlock();
}
}

Expand Down
48 changes: 39 additions & 9 deletions src/main/java/com/fasterxml/jackson/databind/ser/SerializerCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -45,6 +46,8 @@ public final class SerializerCache
*/
private final LookupCache<TypeKey, JsonSerializer<Object>> _sharedMap;

private final ReentrantLock _sharedMapLock = new ReentrantLock();

/**
* Most recent read-only instance, created from _sharedMap, if any.
*/
Expand Down Expand Up @@ -107,29 +110,41 @@ public synchronized int size() {
*/
public JsonSerializer<Object> untypedValueSerializer(Class<?> type)
{
synchronized (this) {
try {
Copy link
Member

@cowtowncoder cowtowncoder Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

_sharedMapLock.lock();
return _sharedMap.get(new TypeKey(type, false));
} finally {
_sharedMapLock.unlock();
}
}

public JsonSerializer<Object> untypedValueSerializer(JavaType type)
{
synchronized (this) {
try {
_sharedMapLock.lock();
return _sharedMap.get(new TypeKey(type, false));
} finally {
_sharedMapLock.unlock();
}
}

public JsonSerializer<Object> typedValueSerializer(JavaType type)
{
synchronized (this) {
try {
_sharedMapLock.lock();
return _sharedMap.get(new TypeKey(type, true));
} finally {
_sharedMapLock.unlock();
}
}

public JsonSerializer<Object> typedValueSerializer(Class<?> cls)
{
synchronized (this) {
try {
_sharedMapLock.lock();
return _sharedMap.get(new TypeKey(cls, true));
} finally {
_sharedMapLock.unlock();
}
}

Expand All @@ -146,29 +161,36 @@ public JsonSerializer<Object> typedValueSerializer(Class<?> cls)
*/
public void addTypedSerializer(JavaType type, JsonSerializer<Object> 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<Object> 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();
}
}

public void addAndResolveNonTypedSerializer(Class<?> type, JsonSerializer<Object> ser,
SerializerProvider provider)
throws JsonMappingException
{
synchronized (this) {
try {
_sharedMapLock.lock();
if (_sharedMap.put(new TypeKey(type, false), ser) == null) {
_readOnlyMap.set(null);
}
Expand All @@ -180,14 +202,17 @@ public void addAndResolveNonTypedSerializer(Class<?> type, JsonSerializer<Object
if (ser instanceof ResolvableSerializer) {
((ResolvableSerializer) ser).resolve(provider);
}
} finally {
_sharedMapLock.unlock();
}
}

public void addAndResolveNonTypedSerializer(JavaType type, JsonSerializer<Object> ser,
SerializerProvider provider)
throws JsonMappingException
{
synchronized (this) {
try {
_sharedMapLock.lock();
if (_sharedMap.put(new TypeKey(type, false), ser) == null) {
_readOnlyMap.set(null);
}
Expand All @@ -199,6 +224,8 @@ public void addAndResolveNonTypedSerializer(JavaType type, JsonSerializer<Object
if (ser instanceof ResolvableSerializer) {
((ResolvableSerializer) ser).resolve(provider);
}
} finally {
_sharedMapLock.unlock();
}
}

Expand All @@ -213,7 +240,8 @@ public void addAndResolveNonTypedSerializer(Class<?> 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)) {
Expand All @@ -222,6 +250,8 @@ public void addAndResolveNonTypedSerializer(Class<?> rawType, JavaType fullType,
if (ser instanceof ResolvableSerializer) {
((ResolvableSerializer) ser).resolve(provider);
}
} finally {
_sharedMapLock.unlock();
}
}

Expand Down
Loading