Transaction is left open when error happens in Datastore API #2297

Activity

1. tseaver

   added

   api: datastoreIssues related to the Datastore API.Issues related to the Datastore API.

   on Sep 12, 2016
2. tseaver commented on Sep 12, 2016

   

   tseaver

   on Sep 12, 2016

   Contributor

   More actions

   Thanks for the report!
3. dhermes commented on Sep 12, 2016

   

   dhermes

   on Sep 12, 2016

   Contributor

   More actions

   I was able to reproduce this, digging in now.
4. dhermes

   self-assigned this

   on Sep 12, 2016
5. dhermes commented on Sep 12, 2016

   

   dhermes

   on Sep 12, 2016

   Last edited by dhermes

   Contributor

   More actions

   After just faking the 500

   diff --git a/google/cloud/datastore/transaction.py b/google/cloud/datastore/transaction.py
   index 66ef8bb..0318643 100644
   --- a/google/cloud/datastore/transaction.py
   +++ b/google/cloud/datastore/transaction.py
   @@ -128,6 +128,10 @@ class Transaction(Batch):
            :raises: :class:`ValueError` if the transaction has already begun.
            """
            super(Transaction, self).begin()
   +        import httplib2
   +        from google.cloud.exceptions import make_exception
   +        resp = httplib2.Response({'status': 500})
   +        raise make_exception(resp, {'error': {'message': 'internal error'}})
            self._id = self.connection.begin_transaction(self.project)
   
        def rollback(self):

   I ran the following:

   >>> from google.cloud import datastore
   >>> client = datastore.Client()
   >>> q = client.query(kind='Foo')
   >>> client.connection._datastore_api
   <google.cloud.datastore.connection._DatastoreAPIOverGRPC object at 0x7f4997d6c390>
   >>> list(q.fetch())
   []
   >>> k = client.key('Foo', 1234)
   >>> e = datastore.Entity(key=k)
   >>> e['food'] = 'noms'
   >>> client.put(e)
   >>> list(q.fetch())
   [<Entity[{'kind': u'Foo', 'id': 1234L}] {u'food': 'noms'}>]
   >>> with client.transaction():
   ...     e['goodbye'] = 123
   ...     client.put(e)
   ... 
   Traceback (most recent call last):
     File "<stdin>", line 1, in <module>
     File ".tox/py27/local/lib/python2.7/site-packages/google/cloud/datastore/batch.py", line 275, in __enter__
       self.begin()
     File ".tox/py27/local/lib/python2.7/site-packages/google/cloud/datastore/transaction.py", line 134, in begin
       raise make_exception(resp, {'error': {'message': 'internal error'}})
   google.cloud.exceptions.InternalServerError: 500 internal error
   >>> list(q.fetch())
   [<Entity[{'kind': u'Foo', 'id': 1234L}] {u'food': 'noms'}>]
   >>> k2 = client.key('Foo', 123456)
   >>> e2 = datastore.Entity(key=k2)
   >>> e2['other'] = 'brother'
   >>> client.put(e2)
   >>> 
   >>> list(q.fetch())
   [<Entity[{'kind': u'Foo', 'id': 1234L}] {u'food': 'noms'}>]
   >>>
   >>> # Just use a FRESH client
   >>> client = datastore.Client()
   >>> client.put(e2)
   >>> list(q.fetch())
   [<Entity[{'kind': u'Foo', 'id': 1234L}] {u'food': 'noms'}>, <Entity[{'kind': u'Foo', 'id': 123456L}] {u'other': 'brother'}>]

   The issue is certainly in Batch.__enter__, hope to have it sorted out soon.
6. dhermes

   added 2 commits that reference this issue on Sep 12, 2016

   Making datastore batch/transaction more robust to failure.

   ...

   This issue will close once commit 98d76c9 is merged into the 'master' branch.

   98d76c9

   Making datastore batch/transaction more robust to failure.

   ...

   This issue will close once commit 6020ee7 is merged into the 'master' branch.

   6020ee7
7. dhermes

   added

   type: bugError or flaw in code with unintended results or allowing sub-optimal usage patterns.Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

   on Sep 12, 2016
8. dhermes commented on Sep 12, 2016

   

   dhermes

   on Sep 12, 2016

   Contributor

   More actions

   @dmho418 It occurred to me while making the fix #2303 that this error would really only be perceivable if you were in the interpreter or some other situation where the uncaught exception didn't hose your entire application. Are you catching errors and recovering somehow? You may want to look into how much "recovery" is done.

   ---

   @tseaver I wanted to make sure that a failure in __enter__ means __exit__ is never called AND that the failure in __enter__ bubbles up to the user:

   >>> class A(object):
   ...     def __enter__(self):
   ...         print('__enter__ called')
   ...         raise ValueError('Bad')
   ...         return self
   ...     def __exit__(self, exc_type, exc_val, exc_tb):
   ...         msg = '__exit__ called: %r, %r, %r' % (exc_type, exc_val, exc_tb)
   ...         print(msg)
   ... 
   >>> 
   >>> a = A()
   >>> with a:
   ...     print('I made it inside the context manager')
   ... 
   __enter__ called
   Traceback (most recent call last):
     File "<stdin>", line 1, in <module>
     File "<stdin>", line 4, in __enter__
   ValueError: Bad
9. dmho418 commented on Sep 16, 2016

   

   dmho418

   on Sep 16, 2016

   Author

   More actions

   @dhermes you got it, I'm catching that exception since I don't want the application to crash. Ideally, the client should remain valid after the exception, right?
10. dhermes commented on Sep 16, 2016

    

    dhermes

    on Sep 16, 2016

    Contributor

    More actions

    Yeah, #2303 resolves that issue. Thanks for bringing it to our attention.
11. dhermes

    closed this as completedin #2303on Sep 16, 2016
12. theacodes

    unassigned

    dhermes

    on Sep 28, 2018
13. JustinBeckwith

    assigned

    dhermes

    on Feb 1, 2021
14. parthea

    added a commit that references this issue on Apr 1, 2026

    refactor: fix ops.StrftimeOp, ops.ToDatetimeOp, ops.ToTimestampOp in …

    ...

    6c612e9