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.

Sign up for GitHub

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

Jump to bottom

Session leak when read-write statement-based transactions are retried #300

Closed
thepaul opened this issue Oct 4, 2024 · 1 comment
Closed

Session leak when read-write statement-based transactions are retried #300

thepaul opened this issue Oct 4, 2024 · 1 comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@thepaul
Copy link
Contributor

thepaul commented Oct 4, 2024

Environment details

  • Programming language: Go
  • OS: macOS, Linux
  • Language runtime version: 1.23.2
  • Package version: 1.7.2

Steps to reproduce

  1. Use the Cloud Spanner emulator as the backend (it will abort queries more readily than the real Spanner).
  2. Start a transaction tx1.
  3. Execute a query (like SELECT 1) in tx1 that will cause a session to be opened. Leave that session open.
  4. Outside of any transaction context, execute another single-statement query, i.e. (SELECT 2).
  5. The SELECT 2 query will be aborted (because the emulator only supports one transaction at a time) and will go into an infinite retry loop using (*readWriteTransaction).retry() waiting for tx1 to close.
  6. Close tx1 (rollback or commit).
  7. If the SELECT 2 query has been retrying for long enough, it will have exhausted the limit on session handles and will block forever waiting for more. SELECT 2 will never complete. Also, no more sessions can be opened at this point.
@thepaul thepaul added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Oct 4, 2024
thepaul added a commit to thepaul/go-sql-spanner that referenced this issue Oct 4, 2024
@thepaul
fix: avoid session leak on transaction retry (googleapis#300)
e39ec1d 
When `(*readWriteTransaction).retry()` is used, there is already an
existing pending (errored) transaction. It gets overwritten with the new
transaction object without closing the old one.

This change causes the old transaction to be closed (rollback'd) before
the new one overwrites it, thus avoiding a potentially large session
leak.
olavloite added a commit to googleapis/google-cloud-go that referenced this issue Oct 7, 2024
@olavloite
feat(spanner): add ResetForRetry method for stmt-based transactions
109f3e3 
Read/write transactions that are aborted should preferably be retried using the
same session as the original attempt. For this, statement-based transactions
should have a ResetForRetry function. This was missing in the Go client library.

This change adds this method, and re-uses the session when possible. If the
aborted error happens during the Commit RPC, the session handle was already
cleaned up by the original implementation. We will not change that now, as
that could lead to breakage in existing code that depends on this. When
the Go client is switched to multiplexed sessions for read/write transactions,
then this implementation should be re-visited, and it should be made sure that
ResetForRetry optimizes the retry attempt for an actual retry.

Updates googleapis/go-sql-spanner#300
@olavloite olavloite mentioned this issue Oct 7, 2024
Merged
gcf-merge-on-green bot pushed a commit that referenced this issue Oct 7, 2024
@thepaul
fix: avoid session leak on transaction retry (#300) (#301)
129248c 
When `(*readWriteTransaction).retry()` is used, there is already an existing pending (errored) transaction. It gets overwritten with the new transaction object without closing the old one.

This change causes the old transaction to be closed (rollback'd) before the new one overwrites it, thus avoiding a potentially large session leak.
@release-please release-please bot mentioned this issue Oct 7, 2024
Merged
@olavloite
Copy link
Collaborator

Fixed in #301. Further optimizations will follow once googleapis/google-cloud-go#10956 is available.

storjBuildBot pushed a commit to storj/storj that referenced this issue Oct 8, 2024
@thepaul
satellite/satellitedb: reenable TestRepairQueue_Select_Concurrently
ffd0a8a 
This test was disabled for Spanner in commit
08a6811 because of a session leak in
go-sql-spanner: googleapis/go-sql-spanner#300

Since we now have the fix for that, I am reenabling this test.

Change-Id: Ide3aae16dc389485ab52f0c741bf1b490340d46e
olavloite added a commit to googleapis/google-cloud-go that referenced this issue Nov 12, 2024
@olavloite @harshachinta
feat(spanner): add ResetForRetry method for stmt-based transactions (#…
02c191c 
…10956)

* feat(spanner): add ResetForRetry method for stmt-based transactions

Read/write transactions that are aborted should preferably be retried using the
same session as the original attempt. For this, statement-based transactions
should have a ResetForRetry function. This was missing in the Go client library.

This change adds this method, and re-uses the session when possible. If the
aborted error happens during the Commit RPC, the session handle was already
cleaned up by the original implementation. We will not change that now, as
that could lead to breakage in existing code that depends on this. When
the Go client is switched to multiplexed sessions for read/write transactions,
then this implementation should be re-visited, and it should be made sure that
ResetForRetry optimizes the retry attempt for an actual retry.

Updates googleapis/go-sql-spanner#300

* fix: only allow resetting if tx is really aborted

---------

Co-authored-by: Sri Harsha CH <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@thepaul @olavloite