Skip to content

fix(spanner): cancel queries promptly on close and synchronize error handling - #14887

Merged
olavloite merged 2 commits into
mainfrom
spanner-early-stream-registration
Nov 2, 2026
Merged

olavloite merged 2 commits into
mainfrom
spanner-early-stream-registration

Conversation

@olavloite

Copy link
Copy Markdown
Contributor

When a query or transaction is cancelled or closed while the network request is still being established, the underlying gRPC stream was previously left unregistered, allowing the query to continue running in the background and wasting database and network resources. Additionally, closing a query concurrently with an error could cause completion callbacks to race ahead of error handlers, corrupting transaction state and masking the underlying failure.

This change fixes several concurrency edge cases in query execution:

  • Prompt query cancellation: Registers streams immediately so that queries cancelled or closed during startup are aborted right away, or skipped entirely if already closed, preventing redundant network RPCs.
  • Clean failure for early closes: When a query that initiates a transaction is closed before the request finishes, waiting callers fail fast with a clear error rather than hanging indefinitely for a transaction ID.
  • Coordinated error and close callbacks: Guarantees that query errors are fully dispatched before completion notifications run, ensuring transactions observe the actual database failure and can retry appropriately instead of being masked by a generic "result set closed" error.
  • Fast failure on repeated reads: Once a query stream encounters an error, subsequent read calls immediately surface the original failure rather than attempting further operations.

@olavloite
olavloite requested review from a team as code owners November 2, 2026 13:34

@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 addresses concurrency issues and race conditions when closing result sets or transactions before a stream starts or a transaction ID is returned. It introduces safe handling of concurrent closures during stream initialization, prevents unnecessary RPC calls, and ensures that onError and onDone callbacks are invoked in the correct order without duplicates. Additionally, comprehensive unit tests are added to verify these concurrent behaviors. Feedback on the changes suggests simplifying a redundant conditional check in AbstractReadContext.java where onClosedBeforeStreamStarted is called.

…handling

When a query or transaction is cancelled or closed while the network request is still being established, the underlying gRPC stream was previously left unregistered, allowing the query to continue running in the background and wasting database and network resources. Additionally, closing a query concurrently with an error could cause completion callbacks to race ahead of error handlers, corrupting transaction state and masking the underlying failure.

This change fixes several concurrency edge cases in query execution:

- Prompt query cancellation: Registers streams immediately so that queries cancelled or closed during startup are aborted right away, or skipped entirely if already closed, preventing redundant network RPCs.
- Clean failure for early closes: When a query that initiates a transaction is closed before the request finishes, waiting callers fail fast with a clear error rather than hanging indefinitely for a transaction ID.
- Coordinated error and close callbacks: Guarantees that query errors are fully dispatched before completion notifications run, ensuring transactions observe the actual database failure and can retry appropriately instead of being masked by a generic "result set closed" error.
- Fast failure on repeated reads: Once a query stream encounters an error, subsequent read calls immediately surface the original failure rather than attempting further operations.
@olavloite
olavloite force-pushed the spanner-early-stream-registration branch from 267a910 to c8c9b3c Compare November 2, 2026 15:13
@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 addresses concurrency issues and race conditions when closing a ResultSet or transaction concurrently with stream initialization or error dispatching in the Google Cloud Spanner Java client. Key changes include tracking stream state via setStream to prevent unnecessary RPC calls if closed concurrently, deferring onDone notifications until onError callbacks complete to avoid race conditions, and caching exceptions in GrpcResultSet to ensure subsequent next() calls consistently throw the same error. Extensive unit tests have been added to verify these concurrent behaviors and prevent regressions. I have no feedback to provide as there are no review comments.

@olavloite
olavloite enabled auto-merge (squash) November 2, 2026 16:33
@olavloite
olavloite merged commit 323a8f7 into main Nov 2, 2026
203 checks passed
@olavloite
olavloite deleted the spanner-early-stream-registration branch November 2, 2026 17:01
zhumin8 added a commit that referenced this pull request Nov 2, 2026
…heck (#14896)

Fix generation diff, unblocks the Librarian upgrade and API sources
regen.

## What
Runs `google-java-format` 1.25.2 on two handwritten test files, so their
long string literals are reflowed the same way `librarian generate
--all` reflows them:

- `java-storage/.../it/ITGrpcDirectPathTest.java`: `@Ignore` message
(added in #14006)
- `java-spanner/.../AbstractReadContextTest.java`: `FAILED_PRECONDITION`
message (added in #14887)

The result is identical to the diff in the failing [generation check
run](https://github.com/googleapis/google-cloud-java/actions/runs/41497486252)
(same blob hashes: `06333bef`, `bc58e312`). These are formatting-only
changes, with no behavior change.

## Why the PRs passed lint but broke `main`
- **PR `lint` job:** runs `fmt-maven-plugin` 2.25. Its `Formatter` calls
`formatSource()` and then only fixes imports. It never reflows long
strings (`StringWrapper.wrap`).
- **`librarian generate`:** runs the `google-java-format --replace` CLI,
where `reflowLongStrings` defaults to `true`. It runs on every `.java`
file in each library, including handwritten tests.
- **`librarian_generation_check.yaml`:** runs only on push to `main`, so
the mismatch shows up only after merge.

Both tools use google-java-format 1.25.2, so the version isn't the
cause. The difference is the long-string reflow step. This will happen
again whenever a handwritten string literal is longer than 100 columns,
until the two formatters agree. That follow-up is out of scope for this
PR.


Fixes #14862
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