Transaction does not get rolled back if exception happens during commit.

For example:

# >>> entity.kind()
# 'Knd'
with datastore.transaction.Transaction():
    # entity1 has an incomplete key, hence the delete fails
    entity1.delete()
# Exception: Request failed. Error was: Key path element must not be incomplete: [Knd: ]

with datastore.transaction.Transaction():
    entity2.save()
# ValueError: Cannot start a transaction with another already in progress.

Maybe this isn't such a big deal since most code will stop working after the first uncaught exception? The only reason I noticed is because using a test-runner, the exception is caught and printed, but the process just moves on to the next.

Hmm, list(query.fetch()) is returning empty, while list(query.fetch(limit=100)) is returning 3 posts.

DJH NOTE: This was in reference to #488

I think 0 is different than not setting in datastore_pb.Query.

DJH NOTE: This was in reference to #488

I just realized this was filed and closed as #136. However, I think it should be addressed as discussed by Tres.

@tseaver

@tseaver This will probably be fixed as part of #514. This only causes failure of future code because the cleanup

self.connection.transaction(None)

doesn't get a chance to occur either in Transaction.commit() or Transaction.rollback().

Once we rely on Batch.__exit__ to pop the current Transaction off the stack in finally:, this bug should go away.

However, it's unclear to me if we need to rollback() a transaction which fails on commit(). It seems like we should, but maybe it doesn't matter.

Working code to re-create this bug (as of 0525530):

>>> from gcloud import datastore
>>> from gcloud.datastore import _implicit_environ
>>> from gcloud.datastore.key import Key
>>> from gcloud.datastore.transaction import Transaction
>>>
>>> print _implicit_environ.CONNECTION
None
>>>
>>> datastore.set_default_connection()
>>> datastore.set_default_dataset_id('foo')
>>>
>>> print _implicit_environ.CONNECTION
<gcloud.datastore.connection.Connection object at 0x7f64c2577190>
>>> print _implicit_environ.CONNECTION.transaction()
None
>>>
>>> with Transaction():
...     partial_key = Key('Knd')
...     partial_key.delete()
...
Traceback (most recent call last):
  File "<stdin>", line 3, in <module>
  File "gcloud/datastore/batch.py", line 277, in __exit__
    self.commit()
  File "gcloud/datastore/transaction.py", line 182, in commit
    super(Transaction, self).commit()
  File "gcloud/datastore/batch.py", line 252, in commit
    response = self.connection.commit(self._dataset_id, self.mutation)
  File "gcloud/datastore/connection.py", line 376, in commit
    datastore_pb.CommitResponse)
  File "gcloud/datastore/connection.py", line 100, in _rpc
    data=request_pb.SerializeToString())
  File "gcloud/datastore/connection.py", line 77, in _request
    raise six.moves.http_client.HTTPException(message)
httplib.HTTPException: Request failed with status code 400. Error was: Key path element must not be incomplete: [Knd: ]
>>> print _implicit_environ.CONNECTION
<gcloud.datastore.connection.Connection object at 0x7f64c2577190>
>>> print _implicit_environ.CONNECTION.transaction()
<gcloud.datastore.transaction.Transaction object at 0x7f64c2464510>

If the back-end returns an error in our call to its commit, I don't think we should be rolling anything back: the "token" for the transaction is already known to be invalid on their side, which is the only reason to call their rollback.

I'm not clear why the "token" for the transaction is known to be valid. I'd like to get input from someone on the datastore team to see if rollback is necessary / a "best practice" in those cases.

rollback exists to allow the application to signal the back-end that it will never call commit, and therefore that the back-end can release any resources for that transaction.

I cannot see any possible way to reuse the opaque transaction field (returned in beginTransaction) after commit API using it fails. The back-end folks will have torn down / released anything they allocated in beginTransaction when the failed the commit, and don't need us to tell them to do so by calling rollback (in fact, I would bet that rollback after a failed commit would error out).