Transactions are commited in CommitRequest.NON_TRANSACTIONAL mode

[mwitkow]

We have just been bitten by this using the latest 0.5.0.

We had the following piece of code, run in two different threads:

with dataset.transaction as t:
  entity = dataset.get(entity_key)
  archived_entity = _move_to_archive_key(entity)
  t.delete(entity.key)
  t.put(archived_entity)
  return archived_entity

We've seen two copies of the archived_entity for the same entity. We've managed to reproduce it with two threads performing this concurrently (running separate Connections each, because httplib2 is not thread safe).

Having used Datastore's underlying technology before, I expected to get a ConcurrentModificationException on delete, and one of the threads throwing it, similar to what the Java API for AppEngine describes:
https://cloud.google.com/appengine/docs/java/datastore/transactions#Java_Uses_for_transactions

However, both of them succeed creating two copies of archived_entity, and there's no ConcurrentModificationException anywhere in gcloud-python code.

We've tracked down to the following:

• Transaction#commit calls the Batch#commit https://github.com/GoogleCloudPlatform/gcloud-python/blob/master/gcloud/datastore/transaction.py#L177
• Batch#commit is unaware of the Transaction#id, calls Connection#commit without the transaction_id parameter https://github.com/GoogleCloudPlatform/gcloud-python/blob/master/gcloud/datastore/batch.py#L214
• Connection#commitgets transaction_id, and sets the REST request's mode to datastore_pb.CommitRequest.NON_TRANSACTIONAL in https://github.com/GoogleCloudPlatform/gcloud-python/blob/master/gcloud/datastore/connection.py#L321

Impact: This means that all transactions made through python-gcloud are non-transactional. Please treat it as a P0 bug.

[mwitkow]

We managed to get the correct behaviour by plumbing through Transaction#id through Batch#commit. The result is that the above test code works as intended: one thread doest the complete transaction, the other one gets:

 File "~/src/python/improbable/db/common/transaction.py", line 15, in commit
   response = self.connection.commit(self._dataset_id, self.mutation, self._id)
 File "~/virtualenv/python/local/lib/python2.7/site-packages/gcloud/datastore/connection.py", line 324, in commit
   datastore_pb.CommitResponse)
 File "~/virtualenv/python/local/lib/python2.7/site-packages/gcloud/datastore/connection.py", line 108, in _rpc
   data=request_pb.SerializeToString())
 File "~/virtualenv/python/local/lib/python2.7/site-packages/gcloud/datastore/connection.py", line 85, in _request
   raise make_exception(headers, content, use_json=False)
Conflict: 409 too much contention on these datastore entities. please try again.

I think the right solution here would be to catch it in the Transaction#commit and rethrow it as ConcurrentModificationException.

[mwitkow]

What's the ETA on a version that supports it? :)

Also, this currently has a very clunky API, since there is no ConcurrentModificationException. This means that the opportunistic concurrency model of Datastore is poorly exposed to the end user.

[dhermes]

@mwitkow-io I'm going to cut a release today (or tomorrow).

[dhermes]

RE: "opportunistic concurrency model of Datastore is poorly exposed to the end user." we expect the error status codes and error messages from the Datastore API (the RPC service) to be useful. If there are issues where they aren't useful, we should take it up with the API folks.

The ConcurrentModificationException in App Engine corresponds to one of these RPC statuses.

[dhermes]

@mwitkow-io Also I'd love to chat about lack of thread-safety in httplib2 and how you address it. Want to hop on a video chat?

We actually don't require use of it, but this is not at all clear from our docs (and may never be).

[dhermes]

@mwitkow-io Also I'd love to chat about lack of thread-safety in httplib2 and how you address it. Want to hop on a video chat?

We actually don't require use of it, but this is not at all clear from our docs (and may never be).

[mwitkow]

Sure thing, would be great to chat!

I'm in London though, so 8h diff to MTV. 5pm tomorrow BST? Please send a
call invite :-)

On Thu, 4 Jun 2015 18:47 Danny Hermes [email protected] wrote:

@mwitkow-io https://github.com/mwitkow-io Also I'd love to chat about
lack of thread-safety in httplib2 and how you address it. Want to hop on
a video chat?

We actually don't require use of it, but this is not at all clear from our
docs (and may never be).

—
Reply to this email directly or view it on GitHub
#903 (comment)
.

[dhermes]

Invite sent for 4pm your time (if you're free). I assumed you use the email listed on your GitHub profile. If 4pm doesn't work I gave you edit privileges on the event.

[mwitkow]

Thanks, that works great :-)

On Thu, 4 Jun 2015 19:16 Danny Hermes [email protected] wrote:

Invite sent for 4pm your time (if you're free). I assumed you use the
email listed on your GitHub profile. If 4pm doesn't work I gave you edit
privileges on the event.

—
Reply to this email directly or view it on GitHub
#903 (comment)
.