search

aaubry / YamlDotNet

YamlDotNet is a .NET library for YAML
MIT License
2.48k stars 466 forks source link

Make DefaultObjectFactory thread safe #920

Open alxmitch opened 3 months ago

alxmitch commented 3 months ago

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.

alxmitch commented 3 months ago

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.

EdwardCooke commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

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