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

[olavloite]

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

[gemini-code-assist]

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]

/gemini review

[gemini-code-assist]

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]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the keep-alive mechanism in ReadWriteTransaction to prevent memory leaks and improve task scheduling. Specifically, it changes KeepAliveRunnable to a static class holding a WeakReference to the transaction, replaces the scheduled executor with a ScheduledThreadPoolExecutor that removes cancelled tasks, and uses abortedLock.tryLock() to reschedule pings if the transaction is busy. Feedback suggests releasing abortedLock before calling maybeScheduleKeepAlivePing() in the exception handler of KeepAliveRunnable.run() to minimize lock scope and prevent potential deadlocks.

[olavloite]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the keep-alive mechanism in ReadWriteTransaction to use a ScheduledThreadPoolExecutor with a remove-on-cancel policy and updates KeepAliveRunnable to use a WeakReference to prevent memory leaks. It also introduces locking to reschedule pings when the transaction is busy and adds corresponding unit tests. The review feedback suggests improving the robustness of the unit tests by asserting the presence of the specific ScheduledFuture in the shared queue rather than asserting the exact queue size, which can cause flakiness during concurrent test execution.

[olavloite]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the keep-alive mechanism in ReadWriteTransaction to prevent memory leaks and queue accumulation. It introduces a ScheduledThreadPoolExecutor with a task-removal-on-cancel policy, refactors KeepAliveRunnable to a static class using a WeakReference to the transaction, and implements non-blocking lock checks to reschedule pings when the transaction is busy. Comprehensive unit tests are also added. The reviewer suggests checking shouldPing() before scheduling a new keep-alive ping to avoid adding redundant tasks to the queue after a transaction has finished.

[olavloite]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the keep-alive mechanism in ReadWriteTransaction to prevent memory leaks and improve concurrency handling. It replaces the single-thread scheduled executor with a ScheduledThreadPoolExecutor configured to remove cancelled tasks, wraps the transaction in a WeakReference inside KeepAliveRunnable, and uses abortedLock.tryLock() to avoid executing pings when the transaction is busy. The review feedback suggests adding @SuppressWarnings("FutureReturnValueIgnored") to prevent static analysis errors, and removing the redundant manual rescheduling logic (along with its associated test) when the lock is busy, as active transactions already schedule pings automatically.

[olavloite]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the keep-alive mechanism in ReadWriteTransaction by replacing the ScheduledExecutorService with a ScheduledThreadPoolExecutor configured to remove cancelled tasks, and converting KeepAliveRunnable to a static class with a WeakReference to prevent memory leaks. It also introduces concurrency controls using abortedLock.tryLock() and adds comprehensive unit tests. The review feedback highlights a critical race condition where failing to acquire abortedLock can permanently terminate the keep-alive loop, and suggests rescheduling the ping when the lock is busy, along with updating the corresponding unit test to assert that a new future is scheduled.

[olavloite]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the keep-alive mechanism in ReadWriteTransaction to prevent memory leaks and improve resource management. Specifically, it replaces the scheduled executor service with a ScheduledThreadPoolExecutor configured to remove tasks on cancellation, and converts KeepAliveRunnable into a static class that holds a WeakReference to the transaction. Additionally, it introduces robust locking and status checks during keep-alive scheduling and execution, accompanied by comprehensive unit tests covering these scenarios. There are no review comments, so I have no feedback to provide.