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

fix: ensure transactions rollback on failure #767

Merged
merged 16 commits into from Sep 29, 2023
Merged

Conversation

daniel-sanche
Copy link
Contributor

Previously, non-retryable exceptions raised during a transaction would be raised immediately, without causing a rollback. This PR adds an extra try...except block to make sure rollback is always called

It also uses exception chaining to resolve actionable error issues for the repo

Fixes: #766, #765

@daniel-sanche daniel-sanche requested review from a team as code owners September 20, 2023 00:10
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: firestore Issues related to the googleapis/python-firestore API. labels Sep 20, 2023
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Sep 25, 2023
@daniel-sanche daniel-sanche changed the title [DRAFT] fix: ensure transactions rollback on failure fix: ensure transactions rollback on failure Sep 25, 2023
@@ -105,14 +105,18 @@ async def _begin(self, retry_id: bytes = None) -> None:
)
self._id = transaction_response.transaction

async def _rollback(self) -> None:
async def _rollback(self, source_exc=None) -> None:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems unfortunate that we have to add an additional parameter to this function. I am guessing the underscore prefix means it is private? If so, I guess it's not that big of a deal, but if this argument is only being used for that one case in the call function then could we catch errors there caused by await transaction._rollback(source_exc=exc) and do raise exc from source_exc there instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not worried about the new parameter because 1) this is a private function, and 2) it's an optional paramater, so existing behaviour would remain consistent

But that's a good point that after this refactor, the rollback only happens in one place. In this case, we can rely on built-in exception chaining instead of raise from, since the exception occurs while handling a different exception. I simplified the code to remove the source_exc

@daniel-sanche daniel-sanche merged commit cdaf25b into main Sep 29, 2023
21 checks passed
@daniel-sanche daniel-sanche deleted the actionable_errors branch September 29, 2023 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/python-firestore API. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

actionable error: some errors related to rpc calls are missing debug info
3 participants