Skip to content

fix(spanner): prevent stream leaks and race conditions on result set close - #14665

Open
olavloite wants to merge 1 commit into
mainfrom
spanner-prevent-stream-leaks
Open

olavloite wants to merge 1 commit into
mainfrom
spanner-prevent-stream-leaks

Conversation

@olavloite

Copy link
Copy Markdown
Contributor

Fixes several race conditions, potential stream leaks, and state loss when closing streaming result sets:

  1. Early Close Race Condition: If ResultSet.close() was called before asynchronous gRPC stream initialization finished (before setCall was invoked), the cancellation was silently dropped because the call reference was still null. When setCall ran moments later, the stream remained open and was never cancelled. Now, closed state is tracked so that setCall immediately cancels the stream if closed early.
  2. Concurrent Close Idempotency: close() is now synchronized and extracts the call reference atomically, guaranteeing call.cancel() is executed at most once, and executing the cancellation outside the lock to prevent deadlocks.
  3. Superfluous Cancellation on Completed Streams: The call handle is now cleared as soon as a PartialResultSet with last=true is consumed, avoiding unnecessary RPC cancellations when closing after reading the final chunk without an extra next() call.
  4. END_OF_STREAM Queue Deduplication: Atomically deduplicate END_OF_STREAM markers to avoid queue overflow or stalls when prefetchChunks is configured to small values (e.g. prefetchChunks = 1).
  5. Inlined Begin Transaction State on Close: In GrpcResultSet.close(), capture withBeginTransaction before closing the underlying iterator so listener.onDone() properly unblocks any threads waiting for inlined transaction IDs.

…close

Fixes several race conditions, potential stream leaks, and state loss when closing streaming result sets:
1. Early Close Race Condition:
   If ResultSet.close() was called before asynchronous gRPC stream
   initialization finished (before setCall was invoked), the cancellation
   was silently dropped because the call reference was still null. When
   setCall ran moments later, the stream remained open and was never cancelled.
   Now, closed state is tracked so that setCall immediately cancels the stream
   if closed early.
2. Concurrent Close Idempotency:
   close() is now synchronized and extracts the call reference atomically,
   guaranteeing call.cancel() is executed at most once, and executing the
   cancellation outside the lock to prevent deadlocks.
3. Superfluous Cancellation on Completed Streams:
   The call handle is now cleared as soon as a PartialResultSet with last=true
   is consumed, avoiding unnecessary RPC cancellations when closing after reading
   the final chunk without an extra next() call.
4. END_OF_STREAM Queue Deduplication:
   Atomically deduplicate END_OF_STREAM markers to avoid queue overflow or stalls
   when prefetchChunks is configured to small values (e.g. prefetchChunks = 1).
5. Inlined Begin Transaction State on Close:
   In GrpcResultSet.close(), capture withBeginTransaction before closing the
   underlying iterator so listener.onDone() properly unblocks any threads waiting
   for inlined transaction IDs.
@olavloite
olavloite requested review from a team as code owners October 16, 2026 13:16

@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 the thread safety and state management of GrpcStreamIterator and GrpcResultSet by introducing thread-safe call handling, improved cancellation logic, and a dedicated Timeout class. Additionally, it adds comprehensive unit tests to verify these changes, including concurrency and inline transaction scenarios. I have no feedback to provide.

This branch has not been deployed

No deployments
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