Repository navigation
fix(spanner): cancel queries promptly on close and synchronize error handling - #14887
Conversation
There was a problem hiding this comment.
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.
267a910 to
c8c9b3c
Compare
|
/gemini review |
There was a problem hiding this comment.
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.
…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
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: