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
Avoid race condition by sending copies of mongo data as op.Data #8
Avoid race condition by sending copies of mongo data as op.Data #8
Conversation
Commit avoids race conditions in tools that depend on GTM by copying the result map[string]interface{}. By doing so, each op's Data is decoupled from each other and will not run into concurrent read/write failures in the downstream library.
Should we do a deep clone here since the data may represent maps of maps? And only if the length of the "mapped" slice is greater than 1? Maybe use encoding/gob since that would give us a deep clone? |
Actually the mgo/bson library already imported can give us a deep clone of the map. |
@zph, I created a branch |
@rwynn I took a look at I don't think it's necessary to bson.Marshal and unmarshal the data in order to get a copy. By using I'd lean towards trying the shallow copy for now and moving to a deep copy solution if it demonstrates a problem. When I tested the shallow copy it no longer caused Your instincts on gob seem good, here's an example of using gob to deep copy data structures I do like the optimization though of checking Want me to include the length check in this PR? Or if you like, you can checkout this branch and commit to it yourself before merging the pull request. |
Also, thanks again for your work on this library :) |
@zph, I think for most use cases, the shallow copy would be fine. I was just trying to solve the general case so that if some other consumer of gtm has docs like { foo: {bar: 1} }, then access of the bar map concurrently would not cause a race. If you think this is overkill, just add the len check to your PR and I'll merge it. |
Skips the shallow copy of map[string]interface{} that's required when multiple ops would otherwise share a datastructure. In circumstances where ops > 1 we perform shallow copy for each. In circumstances where ops == 1 we use current datastructure. By copying data we avoid a data race condition related to concurrent read/write of maps.
I think shallow copy will be sufficient since we don't have more complex or recursive data structures. Trying this out now... |
👍 it looks good in local testing against a staging environment. Ready for merge when you get a chance. Thanks for talking through solutions on this :). |
Commit avoids race conditions in tools that depend on GTM by copying the
result
map[string]interface{}
into a new data structure.By doing so, each op's Data is decoupled from each other and will not run into concurrent read/write failures in the downstream library.
This could be left to downstream libraries to implement but seems preferable to handle here.
Thanks for your continued work on GTM!