Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors AsyncResultSetImpl to replace its blocking CountDownLatch-based synchronization model with a non-blocking, cooperatively scheduled state machine. This change ensures that executor threads (particularly single-threaded executors) are yielded instead of blocked when the buffer is full or when waiting for stream data, effectively preventing potential deadlocks. Additionally, nullability annotations have been integrated, and extensive unit and integration tests have been added to validate the new non-blocking behavior and various concurrency edge cases. I have no further feedback to provide as there are no review comments to assess.
a546877 to
6efbee2
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors AsyncResultSetImpl to implement a non-blocking, event-driven streaming architecture. It replaces the blocking queue with an ArrayDeque and introduces explicit state tracking to allow producer and callback tasks to yield and reschedule on the executor rather than pinning threads when the buffer is full or waiting for stream data. The changes also improve state validation, implement schema methods directly on the result set, correct exception caching during delegate initialization, and add comprehensive concurrency and lifecycle tests. I have no feedback to provide as there are no review comments.
6efbee2 to
b25ad85
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors AsyncResultSetImpl to use a non-blocking, event-driven streaming model, replacing blocking latches with executor yielding and adding comprehensive lifecycle and concurrency tests. The review feedback highlights a performance regression in getOrInitializeDelegate() where the monitor lock is acquired twice on every column read in synchronous (SYNC) mode. To resolve this, it is recommended to cache the initialized delegate in a volatile field to restore lock-free column reads.
Asynchronous queries share a bounded background thread pool to read rows from query streams and dispatch them to application callbacks. Previously, each active query held onto a background thread for its entire lifetime, parking that thread whenever the query was waiting for more data from the server, waiting for a full row buffer to be drained, or paused by the application callback. Running many concurrent or paused queries could exhaust the shared thread pool and stall other queries waiting for an available worker thread. This change switches asynchronous query execution to an event-driven model so background threads are only used while actively moving buffered rows: - Event-driven row production: Background tasks read rows only while stream data is already buffered locally and space remains in the row queue, releasing the worker thread immediately whenever a query is waiting on the network, the row buffer is full, or consumption is paused. Incoming stream data, buffer consumption, and resume calls re-schedule row production as needed. - Serialized stream startup and consumption: Completes stream initialization before row production begins so underlying query iterators are never accessed concurrently across threads, while preserving startup error translation for queries that inline transaction creation. - Prompt cancellation and clean lifecycle termination: Delivers cancellation directly to paused or in-flight callbacks without requiring a separate resume call, closes underlying streams outside internal locks, and waits for all active tasks to finish before completing the result future and releasing executor resources. - Strict callback thread scoping and interrupt handling: Restricts cursor advancement and row reads in asynchronous mode to the active callback invocation thread, and cancels the query while preserving the thread interrupt status if a blocking list conversion is interrupted.
b25ad85 to
2073c07
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors AsyncResultSetImpl to introduce a non-blocking, event-driven streaming model, replacing the blocking queue with an ArrayDeque and optimizing thread utilization. It resolves various concurrency races involving pause, resume, cancellation, and exception propagation, and introduces extensive unit tests covering lifecycle and streaming behaviors. Feedback on the changes highlights a high-severity issue in unwrapGrpcResultSet(), where checking only !delegateInitialized prevents unwrapping and translating startup exceptions for lazily initialized queries.
Asynchronous queries share a bounded background thread pool to read rows from query streams and dispatch them to application callbacks. Previously, each active query held onto a background thread for its entire lifetime, parking that thread whenever the query was waiting for more data from the server, waiting for a full row buffer to be drained, or paused by the application callback. Running many concurrent or paused queries could exhaust the shared thread pool and stall other queries waiting for an available worker thread.
This change switches asynchronous query execution to an event-driven model so background threads are only used while actively moving buffered rows: