jackson-databind icon indicating copy to clipboard operation
jackson-databind copied to clipboard

Remove unnecessary external synchronization from `SerializerCache` access of `_sharedMap`

Open cowtowncoder opened this issue 1 year ago • 1 comments

(note: off-shoot of #4430 )

Due to historical reasons, SerializerCache uses synchronized around all access to LookupCache _sharedMap. This is not necessary as LookupCache implementations must implement thread-safe access. Let's remove these statements around access.

But just in case do it only for 2.18, not 2.17 patch release.

cowtowncoder avatar Mar 22 '24 03:03 cowtowncoder

On second thought... while access in general should not require synchronized, it looks like there might be one big wrinkle -- resolution of cyclic dependencies.

Looking at this:

    public void addAndResolveNonTypedSerializer(Class<?> type, JsonSerializer<Object> ser,
            SerializerProvider provider)
        throws JsonMappingException
    {
        synchronized (this) {
            if (_sharedMap.put(new TypeKey(type, false), ser) == null) {
                _readOnlyMap.set(null);
            }
            // Need resolution to handle cyclic POJO type dependencies
            /* 14-May-2011, tatu: Resolving needs to be done in synchronized manner;
             *   this because while we do need to register instance first, we also must
             *   keep lock until resolution is complete.
             */
            if (ser instanceof ResolvableSerializer) {
                ((ResolvableSerializer) ser).resolve(provider);
            }
        }
    }

I realize that not only does resolve() logic need to be synced for instance, that instance should NOT be exposed before completion via _sharedMap either. So it may actually be necessary to have those synchronized blocks just for this reason: not to protect LookupCache itself (which is thread-safe) but partially initialized BeanSerializers.

Given this I will not proceed with this immediately.

cowtowncoder avatar Mar 22 '24 03:03 cowtowncoder

Not safe to remove, closing.

cowtowncoder avatar Apr 24 '25 04:04 cowtowncoder