Skip to content

test(spanner): unflake ReadWriteTransactionWithInlineBeginTest - #14888

Merged
sakthivelmanii merged 2 commits into
mainfrom
unflake-readwritetransactionwithinlinebegintest
Nov 2, 2026
Merged

sakthivelmanii merged 2 commits into
mainfrom
unflake-readwritetransactionwithinlinebegintest

Conversation

@sakthivelmanii

@sakthivelmanii sakthivelmanii commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Problem

  • The test ReadWriteTransactionWithInlineBeginTest#closeBeforeTransactionIdReturnedDoesNotRaceWithCancelledConsumer synchronizes on mockSpanner.waitForRequestsToContain(ExecuteSqlRequest.class), but the server sees this inside request.start(...) before setCall() and ResumableStreamIterator.setStream() complete, causing a race where resultSet.close() cancels nothing and the consumer thread deadlocks awaiting its own cancellation latch (TimeoutException).

Solution

  • Synchronize directly on the prefetch request() call via streamRegisteredLatch.countDown(), which only executes after setStream() returns, guaranteeing that resultSet.close() cancels the registered stream from the closing thread.
  • Record the consumer thread in consumerThread and only await consumerFinishedNextLatch inside cancel() when invoked from an external thread, preventing self-deadlock if cancel() is ever called on the consumer thread itself.

Synchronize on the prefetch request() call instead of ExecuteSqlRequest mock server receipt to prevent close() from racing with stream registration.
@sakthivelmanii
sakthivelmanii requested review from a team as code owners November 2, 2026 14:20
@sakthivelmanii
sakthivelmanii enabled auto-merge (squash) November 2, 2026 14:20
@sakthivelmanii

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 updates a test in ReadWriteTransactionWithInlineBeginTest.java to use a CountDownLatch (streamRegisteredLatch) to synchronize and wait for the stream to be registered before closing the result set, replacing a wait for mock Spanner requests. However, calling streamRegisteredLatch.await(...) throws a checked InterruptedException which is unhandled in the current lambda context, causing a compilation error. It is recommended to wrap this call in a try-catch block and restore the thread's interrupted status.

@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 updates a test in ReadWriteTransactionWithInlineBeginTest.java to use a CountDownLatch (streamRegisteredLatch) to synchronize the stream registration before closing the result set, replacing a time-based wait for requests. However, calling streamRegisteredLatch.await throws a checked InterruptedException which is not handled inside the lambda expression, leading to a compilation error. It is recommended to wrap this call in a try-catch block, restore the interrupted status, and throw a RuntimeException to ensure the test compiles and fails correctly if interrupted.

@sakthivelmanii

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 improves synchronization in the ReadWriteTransactionWithInlineBeginTest class by introducing a streamRegisteredLatch and tracking the consumerThread to prevent race conditions and self-deadlocks during stream cancellation. The reviewer recommends improving the handling of InterruptedException within the cancel method by throwing a RuntimeException instead of swallowing it, which ensures that the test fails immediately and clearly upon interruption.

@sakthivelmanii
sakthivelmanii merged commit cac1145 into main Nov 2, 2026
203 checks passed
@sakthivelmanii
sakthivelmanii deleted the unflake-readwritetransactionwithinlinebegintest branch November 2, 2026 16:24
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.

2 participants