YamlDotNet
YamlDotNet copied to clipboard
aaubry
Make DefaultObjectFactory thread safe
https://github.com/kubernetes-client/csharp/issues/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.
Given the answer to https://github.com/aaubry/YamlDotNet/discussions/844 (which I was not previously aware of), this change may not be relevant, if ISerializer / IDeserializer are not intended to be thread-safe.
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.
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.
Could you look at the other classes and make sure that they are using concurrent lists and dictionaries as well?
Can you fix the serializationtest conflict? I'll merge in the pr in a couple of weeks.