[olavloite]

Decouple multi-use read-only transactions from internal async result set implementation details by introducing a dedicated lifecycle callback.

Key improvements in this change:

• Replaces the anonymous subclass in read-only transactions with a clean callback that signals when an asynchronous query either begins execution or is closed without starting.
• Lazily resolves the underlying query stream so that closing an async query before it begins execution no longer starts the query or opens an RPC stream, preventing unnecessary network calls and session allocations.
• Permits safe inspection of query statistics and metadata after a result set has been closed.
• Adds comprehensive unit tests for the decoupled lifecycle, post-close inspections, and closed-state error handling.

[gemini-code-assist]

Code Review

This pull request refactors AsyncResultSetImpl to introduce an onStartOrClose callback, allowing callers like MultiUseReadOnlyTransaction to track the query lifecycle more reliably. It simplifies the transaction logic by removing the anonymous class override and tracking state internally within AsyncResultSetImpl using synchronized monitor flags (startOrCloseNotified, delegateInitialized, delegateClosed). Additionally, it ensures that calling next() after closing the result set throws an IllegalStateException, and adds comprehensive unit tests to verify these behaviors. There are no review comments, and the implementation looks solid, so I have no feedback to provide.

[olavloite]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces an onStartOrClose callback to AsyncResultSetImpl to better track the query lifecycle, allowing callers like MultiUseReadOnlyTransaction to simplify pending start tracking. It also refactors delegate initialization and closing logic to prevent resource leaks and adds comprehensive unit tests. The review feedback highlights two important improvements: first, checking this.closed instead of delegateClosed in getDelegate() to prevent unnecessary RPC streams when closed immediately after setting a callback; second, wrapping the onStartOrClose callback execution in a try-catch block to avoid masking primary exceptions.

[olavloite]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors AsyncResultSetImpl to introduce an onStartOrClose callback, allowing callers like MultiUseReadOnlyTransaction to track the query lifecycle more reliably. It simplifies the transaction logic by removing the custom anonymous subclass and replacing it with this callback. Additionally, the changes improve resource management, ensure proper delegate closing, add robust error handling for callback execution, and introduce comprehensive unit tests to verify these behaviors. No review comments were provided, so there is no feedback to address.

[olavloite]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors AsyncResultSetImpl to introduce an onStartOrClose callback, allowing callers like MultiUseReadOnlyTransaction to reliably track the query lifecycle. It improves thread safety by synchronizing listener operations, refactors delegate retrieval and exception handling into a unified getDelegate() method, and adds robust argument validation and resource cleanup. Additionally, the complex tracking logic in MultiUseReadOnlyTransaction is simplified, and comprehensive unit tests are added to cover various lifecycle and edge-case scenarios. I have no further feedback to provide as the changes are well-implemented and thoroughly tested.

[olavloite]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors AsyncResultSetImpl to support an onStartOrClose callback, allowing callers like MultiUseReadOnlyTransaction to track query lifecycles more reliably. It introduces synchronized state variables to manage lazy initialization, early-close reconciliation, and exception propagation, while simplifying transaction tracking. Additionally, extensive unit tests are added to verify these behaviors. The feedback suggests optimizing listener notification in AsyncResultSetImpl by checking if the listeners list is empty before allocating a new ArrayList copy, which avoids unnecessary object allocation in the common case.

[olavloite]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors AsyncResultSetImpl to introduce an onStartOrClose callback for tracking query lifecycle, simplifying the resource tracking logic in MultiUseReadOnlyTransaction. It improves thread safety by synchronizing listener operations and lazily initializing the delegate result set. Feedback suggests catching exceptions thrown by individual listeners during notification to prevent a single failing listener from blocking others.

[olavloite]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors AsyncResultSetImpl to introduce an onStartOrClose lifecycle callback, enabling callers like MultiUseReadOnlyTransaction to track query execution and closure. It adds lazy initialization of the underlying delegate ResultSet and improves synchronization during state transitions. Comprehensive unit tests are also added. However, a critical issue was identified in the close() method: if called during the STREAMING_INITIALIZED state, it fails to close the delegate or trigger the lifecycle callback, potentially causing resource leaks and deadlocks.

[gemini-code-assist]

In the current implementation of close(), closeDelegateResultSet() and maybeNotifyStartOrClose() are only called if the state is INITIALIZED or SYNC. However, if close() is called when the state is STREAMING_INITIALIZED (e.g., immediately after setCallback is called but before the background thread executes), shouldCloseDelegate and shouldNotifyClose will both be false.

This leads to two critical issues:

1. Resource Leak: The delegate ResultSet is never marked as closed (delegateClosed remains false). If a user subsequently calls getMetadata() or getStats(), the delegate will be lazily initialized but never closed, leaking the underlying Spanner session/gRPC stream.
2. Deadlock/Hang: The onStartOrClose callback is never notified. In MultiUseReadOnlyTransaction, this callback is responsible for decrementing the pending starts counter. If it is never called, the transaction will hang indefinitely waiting for the counter to reach zero.

To resolve both issues, close() should unconditionally close the delegate, shut down the executor service (if auto-close is enabled), and notify the lifecycle callback. Additionally, ensure that an explicit null check is performed on the lazily initialized service (ExecutorService) before invoking shutdown() to prevent a NullPointerException.

  @Override
  public void close() {
    synchronized (monitor) {
      if (this.closed) {
        return;
      }
      this.closed = true;
    }
    closeDelegateResultSet();
    if (executorProvider.shouldAutoClose() && service != null) {
      service.shutdown();
    }
    maybeNotifyStartOrClose();
  }

References

1. When using lazily initialized resources (such as ExecutorService), ensure that teardown or close methods perform explicit null checks before invoking methods on them to prevent NullPointerException.