-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Use ReentrantLock
instead of synchronized
in DeserializerCache
to avoid deadlock on pinning
#4430
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.
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
Changes from 2 commits
48124cd
2668e5f
43a2e8a
d512683
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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<T> | |
*/ | ||
protected final DateFormat _customFormat; | ||
|
||
private final ReentrantLock _customFormatLock = new ReentrantLock(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't researched this but I suspect we could find some There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, the existing There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. Choose a reason for hiding this commentThe 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 | ||
*/ | ||
|
@@ -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(); | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same problem as I described for DateDeserializers - what if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that this is wrong. In my opinion the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think However. Since this is not done for correctness but as (possibly misguided?) performance optimization, maybe even use of basic |
||
|
||
/** | ||
* 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<Object> _findDefaultImplDeserializer(Deserializ | |
return NullifyingDeserializer.instance; | ||
} | ||
|
||
synchronized (_defaultImpl) { | ||
try { | ||
_defaultImplLock.lock(); | ||
if (_defaultImplDeserializer == null) { | ||
_defaultImplDeserializer = ctxt.findContextualValueDeserializer( | ||
_defaultImpl, _property); | ||
} | ||
return _defaultImplDeserializer; | ||
} finally { | ||
_defaultImplLock.unlock(); | ||
} | ||
} | ||
|
||
|
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; | ||
|
@@ -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. | ||
*/ | ||
|
@@ -107,29 +110,41 @@ public synchronized int size() { | |
*/ | ||
public JsonSerializer<Object> untypedValueSerializer(Class<?> type) | ||
{ | ||
synchronized (this) { | ||
try { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 In fact... I don't think synchronization should be needed at all here:
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. Choose a reason for hiding this commentThe 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 |
||
_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(); | ||
} | ||
} | ||
|
||
|
@@ -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); | ||
} | ||
|
@@ -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); | ||
} | ||
|
@@ -199,6 +224,8 @@ public void addAndResolveNonTypedSerializer(JavaType type, JsonSerializer<Object | |
if (ser instanceof ResolvableSerializer) { | ||
((ResolvableSerializer) ser).resolve(provider); | ||
} | ||
} finally { | ||
_sharedMapLock.unlock(); | ||
} | ||
} | ||
|
||
|
@@ -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)) { | ||
|
@@ -222,6 +250,8 @@ public void addAndResolveNonTypedSerializer(Class<?> rawType, JavaType fullType, | |
if (ser instanceof ResolvableSerializer) { | ||
((ResolvableSerializer) ser).resolve(provider); | ||
} | ||
} finally { | ||
_sharedMapLock.unlock(); | ||
} | ||
} | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.