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.

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

Remove unnecessary external synchronization from SerializerCache access of _sharedMap #4442

Open
cowtowncoder opened this issue Mar 22, 2024 · 1 comment
Labels

Comments

@cowtowncoder
Copy link
Member

cowtowncoder commented Mar 22, 2024

(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 cowtowncoder added to-evaluate Issue that has been received but not yet evaluated 2.18 and removed to-evaluate Issue that has been received but not yet evaluated labels Mar 22, 2024
@cowtowncoder
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant