Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
dhermes opened this issue Dec 23, 2014 · 10 comments
Closed
Assignees
Labels
api: datastore Issues related to the Datastore API. 🚨 This issue needs some love. triage me I really want to be triaged.

Comments

@dhermes
Copy link
Contributor

dhermes commented Dec 23, 2014

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.

@dhermes dhermes added the api: datastore Issues related to the Datastore API. label Dec 23, 2014
@dhermes dhermes changed the title Transaction does not complete if exception happens during transaction commit. Transaction does not get rolled back if exception happens during commit. Dec 23, 2014
@tseaver
Copy link
Contributor

tseaver commented Jan 6, 2015

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

@dhermes
Copy link
Contributor Author

dhermes commented Jan 6, 2015

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

DJH NOTE: This was in reference to #488

@dhermes
Copy link
Contributor Author

dhermes commented Jan 7, 2015

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

@dhermes
Copy link
Contributor Author

dhermes commented Jan 9, 2015

@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>

@tseaver
Copy link
Contributor

tseaver commented Jan 12, 2015

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.

@dhermes
Copy link
Contributor Author

dhermes commented Jan 12, 2015

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.

@tseaver
Copy link
Contributor

tseaver commented Jan 12, 2015

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).

@dhermes
Copy link
Contributor Author

dhermes commented Jan 12, 2015

I wasn't sure that

will have torn down / released anything they allocated in beginTransaction when
they failed the commit

is actually true. I just tried to commit twice and it clearly fails.

I was "distracted" by the fact that a transaction can have multiple runQuery calls (i.e. read options are set in a query).

@dhermes
Copy link
Contributor Author

dhermes commented Jan 12, 2015

To summarize all that just happened, the only thing left to fix this bug is the removal of

self.connection.transaction(None)

which will happen by the end of #514.

@dhermes
Copy link
Contributor Author

dhermes commented Jan 15, 2015

Fixed as of #550

@dhermes dhermes closed this as completed Jan 15, 2015
@jgeewax jgeewax modified the milestone: Datastore Stable Jan 30, 2015
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. triage me I really want to be triaged. labels Apr 7, 2020
atulep pushed a commit that referenced this issue Apr 3, 2023
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
atulep pushed a commit that referenced this issue Apr 6, 2023
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
atulep pushed a commit that referenced this issue Apr 6, 2023
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
atulep pushed a commit that referenced this issue Apr 18, 2023
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
parthea pushed a commit that referenced this issue Jun 4, 2023
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
parthea added a commit that referenced this issue Aug 15, 2023
This snippet provides an example of how to extract SessionInfo from
WebhookRequests.

Co-authored-by: Anthonios Partheniou <[email protected]>
parthea pushed a commit that referenced this issue Sep 22, 2023
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
parthea pushed a commit that referenced this issue Oct 21, 2023
- [ ] Regenerate this pull request now.

PiperOrigin-RevId: 459095142

Source-Link: googleapis/googleapis@4f1be99

Source-Link: googleapis/googleapis-gen@ae686d9
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiYWU2ODZkOWNkZTRmYzNlMzZkMGFjMDJlZmI4NjQzYjE1ODkwYzFlZCJ9

feat: add audience parameter
PiperOrigin-RevId: 456827138

Source-Link: googleapis/googleapis@23f1a15

Source-Link: googleapis/googleapis-gen@4075a85
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiNDA3NWE4NTE0ZjY3NjY5MWVjMTU2Njg4YTViYmYxODNhYTk4OTNjZSJ9
parthea added a commit that referenced this issue Oct 21, 2023
* fix(deps): allow protobuf 3.19.5

* explicitly exclude protobuf 4.21.0
parthea added a commit that referenced this issue Oct 21, 2023
* fix(deps): Require google-api-core >=1.34.0, >=2.11.0

fix: Drop usage of pkg_resources

fix: Fix timeout default values

docs(samples): Snippetgen should call await on the operation coroutine before calling result

PiperOrigin-RevId: 493260409

Source-Link: googleapis/googleapis@fea4387

Source-Link: googleapis/googleapis-gen@387b734
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiMzg3YjczNDRjNzUyOWVlNDRiZTg0ZTYxM2IxOWE4MjA1MDhjNjEyYiJ9

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* add gapic_version.py

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Co-authored-by: Anthonios Partheniou <[email protected]>
parthea pushed a commit that referenced this issue Oct 22, 2023
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore API. 🚨 This issue needs some love. triage me I really want to be triaged.
Projects
None yet
Development

No branches or pull requests

4 participants