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

Merged
merged 3 commits into from
Aug 31, 2024

Conversation

alxmitch
Copy link
Contributor

@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
Contributor 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?

@EdwardCooke
Copy link
Collaborator

Can you fix the serializationtest conflict? I'll merge in the pr in a couple of weeks.

@EdwardCooke EdwardCooke merged commit efcd9c6 into aaubry:master Aug 31, 2024
1 check passed
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