Skip to content

fix(spanner): prevent memory leak and thread blocking in transaction keep-alive - #13897

Open
olavloite wants to merge 1 commit into
mainfrom
spanner/connection-tx-keep-alive-fix
Open

fix(spanner): prevent memory leak and thread blocking in transaction keep-alive#13897
olavloite wants to merge 1 commit into
mainfrom
spanner/connection-tx-keep-alive-fix

Conversation

@olavloite

Copy link
Copy Markdown
Contributor
  • Enable setRemoveOnCancelPolicy(true) on KEEP_ALIVE_SERVICE so canceled tasks are immediately purged from DelayedWorkQueue.
  • Use a WeakReference<ReadWriteTransaction> in KeepAliveRunnable to prevent scheduled tasks from retaining strong references to transaction instances.
  • Use abortedLock.tryLock() in KeepAliveRunnable so the shared executor thread does not block when a transaction is active or retrying.
  • Remove duplicate maybeScheduleKeepAlivePing listener registration on keep-alive query completion.
  • Add unit tests in ReadWriteTransactionTest verifying task removal on cancel, weak reference retention, non-blocking lock handling, and single ping scheduling on completion.

@olavloite
olavloite requested review from a team as code owners July 27, 2026 09:10

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the transaction keep-alive mechanism in ReadWriteTransaction by using a ScheduledThreadPoolExecutor with removeOnCancelPolicy enabled, and converting KeepAliveRunnable into a static nested class with a WeakReference to prevent memory leaks. It also introduces lock-based synchronization using abortedLock to avoid sending keep-alive pings when the transaction is busy. Feedback on this PR highlights a critical issue where the keep-alive loop can silently terminate when the transaction is busy; because keepAliveFuture.isDone() is still false during execution, the synchronous call to reschedule the ping results in a no-op. A fix is suggested to reset keepAliveFuture to null before rescheduling.

@olavloite
olavloite force-pushed the spanner/connection-tx-keep-alive-fix branch from d5e0904 to 8c8c44d Compare July 28, 2026 07:52
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the keep-alive mechanism in ReadWriteTransaction to prevent memory leaks and improve task cancellation. It changes KeepAliveRunnable to a static class using a WeakReference to the transaction, replaces the scheduled executor with a ScheduledThreadPoolExecutor configured to remove cancelled tasks, and introduces a rescheduling mechanism when the transaction is busy. A critical issue was identified in the newly added test testKeepAliveRescheduledWhenLockBusy, where the use of a ReentrantLock on the same thread allows tryLock() to succeed due to reentrancy, thereby failing to test the actual rescheduling path. The lock should be acquired on a separate thread to correctly simulate a busy transaction.

@olavloite
olavloite force-pushed the spanner/connection-tx-keep-alive-fix branch 3 times, most recently from 8f18b7e to 8b5aaa6 Compare July 28, 2026 08:48
…keep-alive

- Enable `setRemoveOnCancelPolicy(true)` on `KEEP_ALIVE_SERVICE` so canceled tasks are immediately purged from `DelayedWorkQueue`.
- Use a `WeakReference<ReadWriteTransaction>` in `KeepAliveRunnable` to prevent scheduled tasks from retaining strong references to transaction instances.
- Use `abortedLock.tryLock()` in `KeepAliveRunnable` so the shared executor thread does not block when a transaction is active or retrying.
- Remove duplicate `maybeScheduleKeepAlivePing` listener registration on keep-alive query completion.
- Add unit tests in `ReadWriteTransactionTest` verifying task removal on cancel, weak reference retention, non-blocking lock handling, and single ping scheduling on completion.
@olavloite
olavloite force-pushed the spanner/connection-tx-keep-alive-fix branch from 8b5aaa6 to ef6529f Compare July 28, 2026 09:27
@olavloite
olavloite requested a review from sakthivelmanii July 28, 2026 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant