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

Flush readonly map together with shared on SerializerCache.flush() #3791

Merged

Conversation

vdaniloff
Copy link
Contributor

Without these flushing the cache may be not enough - for example if a new subtype for existing interface is added by a new provider, serialization cache will still return old serializer for an interface.

@cowtowncoder
Copy link
Member

cowtowncoder commented Feb 21, 2023

I would need to have some kind of indication of an observed problem to change something that seems to have been working for a decade now... :)

That is, without a test I don't think I'd want to merge this.

I am also not sure what the problem would be: subtypes are not really relevant here as the cache is keyed by actual type, not supertype.

@cowtowncoder
Copy link
Member

Actually, looking at this again, this makes sense. And while it'd be nice to have tests I don't think they are essential after all.
Merging.

@cowtowncoder cowtowncoder merged commit 508efeb into FasterXML:2.15 Apr 6, 2023
@cowtowncoder cowtowncoder added this to the 2.15.0-rc3 milestone Apr 6, 2023
@cowtowncoder cowtowncoder changed the title Flush readonly map together with shared on serialization cache flush Flush readonly map together with shared on SerializerCache.flush() Apr 6, 2023
cowtowncoder added a commit that referenced this pull request Apr 6, 2023
@vdaniloff
Copy link
Contributor Author

vdaniloff commented Apr 7, 2023

Thanks for merging this.

I am really sorry for late answer, forgot about this. After creating the issue I realized it did not help with my original problem anyway (there was a need to make Jackson rebuild polymorphic serializers, after serializer was used before new subclasses where registered). I fixed that problem by changing application.

The change makes some sense, but flushing serializers may still not work:

  1. There a _knownSerializers field in StdSerializerProvider. It is final and if a serializer is already there - it will not be flushed
  2. There's anyway no such method for flush deserialisers in provider (only in DeserializerCache itself)

Combined makes me think maybe there's still something to do. I am not sure now what is the purpose of the method - to really make Jackson recreate serializers (potentially with new logic) or just to free memory (maybe then javadoc update is needed).

@cowtowncoder
Copy link
Member

@vdaniloff Ok thank you for additional information. I am also not sure how useful this method really is; it generally is better to just create a new ObjectMapper, to be honest (f.ex ObjectMapper.copy()).
Part wrt (Std)SerializerProvider definitely seem to make cache flush operation about useless.

But as to original intent I am pretty sure it's about saving memory and not so much new functionality.

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

Successfully merging this pull request may close these issues.

2 participants