search

elodina / go-avro

Apache Avro for Golang
http://elodina.github.io/go-avro/
Apache License 2.0
129 stars 55 forks source link

Fix reflectMap data race #70

Closed serejja closed 8 years ago

serejja commented 8 years ago

Existing reflectMap in datum_utils yields data races when run with -race flag.

This PR introduces RWMutex to allow for multiple readers when the reflectMap is just being read, but locks exclusively on updates.

@crast would like to hear your opinion on this. You added this optimization some time ago but the situation here is quite similar to #69

crast commented 8 years ago

It makes sense from reading your PR - interestingly I can't get the data race to trigger on OSX/x64 - could be luck or it could be an OSX specific deal. But I think it does make sense to put locks around the map for safety.

I think long term, there's actually a better situation we can end up with which involves less locking... or at minimum more granular locking. It's something I mentioned in my third point of this comment - the ability for the user to tell the library to 'compile' the schema and then on the compiled schema perform all optimizations, because at that point the schema is now immutable and you can compute optimized bits (like enum type maps) without worrying the user can change the value or using some special attribute like the enum's name which is not guaranteed to be unique. Also many things can be compiled in advance without locking.

I have prototyped it here: https://github.com/crast/avro/commit/immutable-schema but it doesn't have enough tests so I haven't submitted a PR yet.

serejja commented 8 years ago

Cool, I've taken a look at your changes and definitely makes sense to me, willing to try it out and merge once you post a PR :)

Thanks!

crast commented 8 years ago

@serejja would you take tests written with an external helpers package either testify or GoConvey ?

serejja commented 8 years ago

Yea, I like testify and thought of rewriting existing tests using testify too