Repository navigation
fix(spanner): synchronize stream lifecycle and retry buffering in ResumableStreamIterator - #14770
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a single-consumer threading contract to ResumableStreamIterator and AbstractResultSet, adding robust synchronization and state management to safely handle concurrent closures, lazy stream initialization, and asynchronous polling. Additionally, the internal buffer was migrated from LinkedList to ArrayDeque. The review feedback highlights two critical issues: first, startFailure is not cleared during resetStream(), which breaks the retry mechanism for startup failures; second, concurrent closures can cause uncaught exceptions to propagate from readNextFromStreamIntoBuffer when polling the underlying stream.
…umableStreamIterator Asynchronous result sets interact with ResumableStreamIterator from multiple threads: the caller thread initiating or closing the stream, gRPC callback threads notifying that messages or errors have arrived, and consumer threads polling for readiness and reading rows. Previously, stream lifecycle state and the retry buffer were not synchronized across these interactions, which caused several issues: 1. Duplicate or prematurely consumed streams during startup: When a gRPC callback fired while the initial stream was still being started and prefetched, the consumer thread could observe a partially initialized stream or start a second stream concurrently. Conversely, if a retryable error occurred immediately and the consumer thread reset the stream to retry, the finishing startup call could overwrite the reset state with the failed stream. 2. Uncancelled streams and deadlocks on concurrent close: Closing a result set while a stream was being created or retried could leave the newly created stream open, close it multiple times, or hold an internal lock while invoking external close callbacks. In addition, transaction metadata flags (whether the stream included an inlined BeginTransaction or was the last statement) were lost when the active stream reference was cleared on close or during retry backoff, preventing callers from properly cleaning up transactions and sessions. 3. Premature readiness signals for unresumable chunks: ResumableStreamIterator buffers chunks that do not carry a resume token so that a transient failure can be retried transparently without yielding duplicate data. However, non-blocking readiness checks delegated directly to the underlying gRPC stream and reported data as available as soon as a single tokenless chunk arrived. This caused asynchronous consumers to wake up and block on network I/O waiting for a resume token or buffer limit. How this change fixes these problems: - Coordinates stream startup so only one thread starts a stream at a time while concurrent callers wait for startup to complete, and tracks an in-flight stream separately from the published stream. A stream is only exposed to consumers and readiness checks after startup and initial prefetching have completed, while still allowing concurrent close calls to cancel an in-flight stream immediately and tearing down streams outside the lock. - Preserves stream transaction flags across stream resets, startup failures, and concurrent closes so upper layers always know whether an inlined transaction or final statement was in progress, and ensures the tracing span is always ended exactly once. - Drains available non-error chunks into the internal retry buffer during non-blocking readiness checks and only reports data as ready to poll when the buffer can actually be flushed (a resume token has been seen, the buffer is full, or the stream has reached the end), or when an error or closure needs to be surfaced.
a264732 to
d6a4474
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces thread-safety improvements and documents a single-consumer threading contract for ResumableStreamIterator and AbstractResultSet. Key changes include replacing LinkedList with ArrayDeque for buffering, introducing fine-grained synchronization via streamLock and buffer locks, and adding volatile fields to safely manage stream state transitions, lazy initialization, and concurrent cancellation. Additionally, JSpecify nullness annotations have been integrated. There are no review comments to assess, and I have no further feedback to provide on these changes.
…14885) Merging PR #14770 and PR #14776 concurrently broke the build on main. One PR added unit tests that configure mock streams using a two-argument setCall method to indicate whether a transaction is inlined, while the other PR removed that parameter because production code now configures this during stream construction. This change restores the two-argument setCall method for testing and keeps the inlined transaction state configurable during test setup.
🤖 I have created a release *beep* *boop* --- <details><summary>1.95.0</summary> ## [1.95.0](v1.94.0...v1.95.0) (2026-11-02) ### Features * **bigquery-jdbc:** expose BigQueryResultSetMetadata and getField(int) ([#14761](#14761)) ([f01245a](f01245a)) * **gax:** align resumable upload chunk size to server granularity and surface effective chunk size ([#14670](#14670)) ([6cbb096](6cbb096)) * **google/cloud/designcenter/v1:** add designcenter ([#14823](#14823)) ([01d27dc](01d27dc)) * **google/cloud/observability/v1:** add observability ([#14722](#14722)) ([624f28e](624f28e)) * **oauth2:** support certificate-bound OAuth2 tokens and mTLS rotation for Workload Identity Federation ([#14781](#14781)) ([dcb29b0](dcb29b0)) * **storage:** add bucket IP filtering samples and tests ([#14328](#14328)) ([cb2b726](cb2b726)) * **storage:** add support for DirectPath over Interconnect ([#14006](#14006)) ([ad0eb94](ad0eb94)) * **storage:** OpenTelemetry client metrics configuration and options ([#14484](#14484)) ([7c29e48](7c29e48)) * **storage:** OpenTelemetry client metrics instrument registry and views ([#14398](#14398)) ([60233b7](60233b7)) ### Bug Fixes * add user agent to session path ([#14835](#14835)) ([a653638](a653638)) * **auth:** align mTLS transport and bound token certificate startup polling and well-known discovery ([#14748](#14748)) ([0bae268](0bae268)) * **bigquery-jdbc:** expand handling to OOM errors ([#14759](#14759)) ([7d4b921](7d4b921)) * **bigquery-jdbc:** fix naive fallback of parameter count ([#14779](#14779)) ([c3e4a5b](c3e4a5b)) * **bigquery-jdbc:** format RANGE bounds consistently across REST and Arrow ([#14807](#14807)) ([2108b8e](2108b8e)) * **bigquery-jdbc:** parse scientific-notation epoch TIMESTAMPs ([#14777](#14777)) ([71a31b1](71a31b1)) * **bigquery-jdbc:** preserve pre-1582 and pre-standard-time DATE/DATETIME digits ([#14787](#14787)) ([3dc394f](3dc394f)) * **bigtable:** avoid mutating Query in-place in createPointReadCallable ([#14797](#14797)) ([1b1c6a7](1b1c6a7)) * **bigtable:** check isOk and getRejected first in RetryingVRpc.shouldRetry ([#14795](#14795)) ([d821bd3](d821bd3)) * **bigtable:** check remaining deadline on client-initiated retries in RetryingVRpc ([#14796](#14796)) ([a04e24c](a04e24c)) * **bigtable:** Fix empty RowResult semantics in TestProxy. ([#14810](#14810)) ([1d2fb9a](1d2fb9a)) * **bigtable:** include reversed in Query equals, hashCode, and toString ([#14799](#14799)) ([fe9232b](fe9232b)) * **bigtable:** pass cell value size to RowBuilder.startCell in RowBuilderShim ([#14800](#14800)) ([cfa7949](cfa7949)) * **bigtable:** reject empty split points in RowSetUtil and clarify QueryPaginator Javadoc ([#14811](#14811)) ([1bc507f](1bc507f)) * **bigtable:** remove legacy PollingConfiguration migration in ClientConfigurationManager ([#14822](#14822)) ([95ead45](95ead45)) * **bigtable:** resolve session diversion through a single shared rule ([#14738](#14738)) ([00f7bea](00f7bea)) * **bigtable:** respect query limit smaller than pageSize and avoid mutating Query in QueryPaginator ([#14821](#14821)) ([cecd09e](cecd09e)) * **firestore:** graduate pipeline search from preview to GA ([#14741](#14741)) ([a993026](a993026)) * **gax:** do not treat a SocketTimeoutException as a thread interrupt ([#14778](#14778)) ([7b91805](7b91805)) * **jdbc:** populate `ResultSetMetaData` on empty `DatabaseMetaData` resultsets ([#14809](#14809)) ([4c6a7e3](4c6a7e3)) * **pubsub:** make publish hedging robust to clock jumps, queue errors, and shutdown ([#14860](#14860)) ([9b9e2a5](9b9e2a5)) * **spanner:** attach routing hints to read-write transaction requests ([#14786](#14786)) ([54e0585](54e0585)) * **spanner:** cancel queries promptly on close and synchronize error handling ([#14887](#14887)) ([323a8f7](323a8f7)) * **spanner:** do not cache retryable errors in AbstractLazyInitializer ([#14752](#14752)) ([6a9ac76](6a9ac76)) * **spanner:** restore setCall overload for stream test compatibility ([#14885](#14885)) ([cfe728e](cfe728e)) * **spanner:** synchronize stream lifecycle and retry buffering in ResumableStreamIterator ([#14770](#14770)) ([4edefc5](4edefc5)) * **spanner:** trigger best-effort rollback on indeterminate commit failures ([#14831](#14831)) ([b9d1b95](b9d1b95)) * **storage:** preserve HTTP status for XML multipart upload errors so retries work ([#14804](#14804)) ([3c03034](3c03034)) * **storage:** send destination on gRPC copy when target BlobInfo is provided ([#14784](#14784)) ([43a67a0](43a67a0)) * **storage:** validate CRC32C on HTTP downloads that were not decompressed ([#14805](#14805)) ([61aa22f](61aa22f)) ### Documentation * **bigtable:** document that skipped large rows count towards row limit ([#14801](#14801)) ([91d60e0](91d60e0)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Asynchronous result sets interact with ResumableStreamIterator from multiple threads: the caller thread initiating or closing the stream, gRPC callback threads notifying that messages or errors have arrived, and consumer threads polling for readiness and reading rows. Previously, stream lifecycle state and the retry buffer were not synchronized across these interactions, which caused several issues:
Duplicate or prematurely consumed streams during startup: When a gRPC callback fired while the initial stream was still being started and prefetched, the consumer thread could observe a partially initialized stream or start a second stream concurrently. Conversely, if a retryable error occurred immediately and the consumer thread reset the stream to retry, the finishing startup call could overwrite the reset state with the failed stream.
Uncancelled streams and deadlocks on concurrent close: Closing a result set while a stream was being created or retried could leave the newly created stream open, close it multiple times, or hold an internal lock while invoking external close callbacks. In addition, transaction metadata flags (whether the stream included an inlined BeginTransaction or was the last statement) were lost when the active stream reference was cleared on close or during retry backoff, preventing callers from properly cleaning up transactions and sessions.
Premature readiness signals for unresumable chunks: ResumableStreamIterator buffers chunks that do not carry a resume token so that a transient failure can be retried transparently without yielding duplicate data. However, non-blocking readiness checks delegated directly to the underlying gRPC stream and reported data as available as soon as a single tokenless chunk arrived. This caused asynchronous consumers to wake up and block on network I/O waiting for a resume token or buffer limit.
How this change fixes these problems: