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

Make DefaultObjectFactory thread safe #920

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

alxmitch
Copy link

@alxmitch alxmitch commented Apr 2, 2024

kubernetes-client/csharp#1537 raised that deserialization (and serialization) is not currently thread-safe, due to concurrent access to the non-thread-safe _stateMethods dictionary in DefaultObjectFactory.

This PR changes the nested inner Dictionary<Type, MethodInfo[]>s in _stateMethods to ConcurrentDictionarys to make concurrent access safe.

Testing

I've added tests to concurrently serialize and deserialize simple YAML. These reproduced the issue before the fix most of the time, but were not 100% reliable (i.e. occasionally suffered false-positive results). I'm open to suggestions for a better way of testing this for thread-safety.

@alxmitch alxmitch marked this pull request as ready for review April 3, 2024 01:51
@alxmitch
Copy link
Author

alxmitch commented Apr 3, 2024

Given the answer to #844 (which I was not previously aware of), this change may not be relevant, if ISerializer / IDeserializer are not intended to be thread-safe.

@EdwardCooke
Copy link
Collaborator

To be able to use the same serializer/deserializer work in multiple threads it’s a lot more than just that dictionary. There’s tons of instance fields that are used through the classes like the emitter and parser.

@EdwardCooke
Copy link
Collaborator

This library is more thread safe than I originally thought. I’m on my phone but going through the classes real quick the anchor related classes seem to be the remaining non-thread safe areas. I didn’t do much research, like where they’re newed up to see if it even matters, or any other digging into the guts of the library. It would be interesting to run your tests with a much larger yaml payload. Like what’s in the benchmark project.

@EdwardCooke
Copy link
Collaborator

Could you look at the other classes and make sure that they are using concurrent lists and dictionaries as well?

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.

None yet

2 participants