Transaction "add"s are not multi-thread safe

[rich-j]

The add(entity) method in a Transaction object is not multi-thread safe. Other methods (e.g. put, delete) may also not be safe but we have traced a failure in our server specifically to add. Our server uses Scala Futures and so uses multiple threads of execution.

The particular scenario we tracked down was in our new user creation processing where we add 4 entities in a transaction. However there was a probability that one of the entities didn't get saved. It wasn't always the same entity kind that "disappeared". We traced this by adding debug logging after the 'add' calls that shows the adds successfully completing - but subsequent processing would find one of the entities missing. We did note that it always seemed to be just one entity missing and when an entity did go missing it seemed to be the first entity kind to be added (thread execution order was not consistent across invocations).

We added synchronize on each of our DataStore calls and haven't encountered this issue since. So at a minimum this issue can serve as a warning to others. Maybe a note could be added to the documentation. But we are also patiently waiting for issue #22 (async support) to be implemented and would hope that explicit synchronization to not be needed.

[crwilcox]

seems like we may want to at minimum document this. I am unsure if they are intended to be used in this way and if there aren't other issues or not. @kolea2 is multithread usage of a client a supported scenario for java clients?

[Neenu1995]

@kolea2 ping

[jainsahab]

I was able to replicate this using this code. A transaction getting modified multiple times through 4 threads. To successfully replicate, this needs to run in loop as it's nondeterministic behaviour.

Digging deeper into this, Transaction class maintains an internal state in terms of LinkedHashMap which gets updated on every Transaction#add call to record the mutations, I also want to highlight the fact that LinkedHashMap is not thread safe as per its documentation.

Note that this implementation is not synchronized. If multiple threads access a linked hash map concurrently, and at least one of the threads modifies the map structurally, it must be synchronized externally. This is typically accomplished by synchronizing on some object that naturally encapsulates the map. If no such object exists, the map should be "wrapped" using the Collections.synchronizedMap method. This is best done at creation time, to prevent accidental unsynchronized access to the map:

There are higher chances that Transaction#put, Transaction#update and Transaction#delete are carrying this bug too, as they also uses same/similar data structures internally.

I feel this is by design as the early developers of the library wanted to access the Transaction object through only one thread to keep things simple. May be the intention was to never access the Transaction object from multiple threads.

For now, documenting the thread unsafe behaviour of Transaction, Batch and DatastoreBatchWriter classes to have users be aware of the existing behaviour.