search

googleapis / google-cloud-go

Google Cloud Client Libraries for Go.
https://cloud.google.com/go/docs/reference
Apache License 2.0
3.8k stars 1.31k forks source link

datastore: Transaction appears to be unsafe to use concurrently #3750

Open riannucci opened 3 years ago

riannucci commented 3 years ago

Client

Datastore

Environment

Any

Go Environment

Any

Code

e.g. https://github.com/googleapis/google-cloud-go/blob/master/datastore/transaction.go#L311

At least the mutations field is unguarded by any synchronization mechanism, but the pending field is also suspect.

This is a seeming departure from the GAE version of the library e.g. (https://github.com/golang/appengine/blob/master/datastore/datastore.go#L351) where Put requests translated directly to a (concurrency safe) RPC which included the transaction ID as part of the RPC. Direct translations of older GAE code to use the new datastore library could introduce subtle concurrency bugs.

I believe adding mutex would be sufficient to protect the Transaction struct's integrity... however if this is infeasible, it would be great to explicitly call out on Transaction that it is NOT concurrency-safe.

riannucci commented 3 years ago

FWIW, we were seeing "spooky" panics (deep inside protobuf marshaling code) when doing multiple Put operations in a Transaction concurrently; wrapping the Transaction with a small struct to enforce a Mutex was enough to remedy the situation (https://chromium.googlesource.com/infra/luci/luci-go/+diff/60b139c..a082a88/gae/impl/cloud)