Repository navigation
fix(spanner): reset transaction flag when session checkout fails - #19014
Open
founderinaction wants to merge 1 commit into
Open
founderinaction wants to merge 1 commit into
founderinaction wants to merge 1 commit into
Conversation
`Database.run_in_transaction` set `_local.transaction_running` before checking out a session, outside the `try`/`finally` that resets it. If `get_session()` raised (for example `ServiceUnavailable` while Spanner is down), the flag stayed set and every later call on that thread failed with "Spanner does not support nested transactions". Check out the session inside the `try`, and only return it to the sessions manager when one was obtained. The async implementation had the same shape and is fixed the same way; the sync module is regenerated output of it. Fixes googleapis#18375
Contributor
There was a problem hiding this comment.
Code Review
This pull request modifies the run_in_transaction method in both the synchronous and asynchronous database implementations to initialize the session variable to None and safely retrieve it within the try block. In the finally block, the session is only returned to the session manager if it was successfully created (i.e., is not None). This prevents secondary errors when session retrieval fails. Corresponding unit tests have been added to verify error handling during session retrieval and callback execution. There are no review comments, so I have no feedback to provide.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #18375 🦕
Database.run_in_transactionsets_local.transaction_running = Trueand then callsget_session()outside thetry/finallythat resets the flag. If checkout raises, for exampleServiceUnavailablewhile Spanner is down, the flag is never cleared. Every later call on that thread then fails withRuntimeError: Spanner does not support nested transactions, even after Spanner recovers.The change moves session checkout inside the
try, and only callsput_sessionwhen a session was actually obtained, soNoneis never returned to the sessions manager. The async implementation in_async/database.pyhad the same shape. I fixed it there and made the identical change to the generated sync module, and checked that the CrossSync conversion of the async method matches the sync one.Tests, sync and async:
test_run_in_transaction_session_error: the first checkout raisesServiceUnavailable, and a second call succeeds. The failed attempt doesn't callput_session, the flag is reset, and the session from the second call is returned.test_run_in_transaction_callback_error: when the callback raises, the flag is reset and the session is still returned. This pins the existing behaviour.Without the fix, both
session_errortests fail with the nested-transactionRuntimeError. With it,tests/unitpasses, and ruff and flake8 are clean.@mukeshr06 reproduced this in September and mentioned preparing a fix. I haven't seen a PR since, so here is one. Happy to close this in favour of theirs if it's close.