chore(storage): exposed fastOpenBidiRead to Storage interface.#13021
chore(storage): exposed fastOpenBidiRead to Storage interface.#13021ShreyasSinha wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements a 'fast-open' version of the blobReadSession API, allowing the initial read to start concurrently with the session opening. The changes include updates to BlobReadSessionAdapter to manage pre-opened streams, implementation in GrpcStorageImpl, and telemetry support in OtelStorageDecorator. Feedback identifies a potential span leak in OtelStorageDecorator during synchronous failures and suggests refactoring BlobReadSessionAdapter to address logic duplication and prevent resource leaks when the fast-open configuration does not match.
| Span ready = tracer.spanBuilder(BLOB_READ_SESSION + "/ready").startSpan(); | ||
| ApiFuture<BlobReadSession> blobReadSessionApiFuture = | ||
| delegate.blobReadSession(id, config, options); |
There was a problem hiding this comment.
The ready span is started but not ended if delegate.blobReadSession throws a synchronous exception (for example, if the transport is not gRPC and throwGrpcOnly is called). This results in a span leak. The call to the delegate should be wrapped in a try-catch block to ensure the ready span is properly recorded and ended in case of an immediate failure.
Span ready = tracer.spanBuilder(BLOB_READ_SESSION + "/ready").startSpan();
ApiFuture<BlobReadSession> blobReadSessionApiFuture;
try {
blobReadSessionApiFuture = delegate.blobReadSession(id, config, options);
} catch (Throwable t) {
ready.recordException(t);
ready.setStatus(StatusCode.ERROR, t.getClass().getSimpleName());
ready.end();
throw t;
}References
- Ensure closeable resources (like spans/scopes) are managed in an exception-safe manner to prevent resource leaks, ensuring they are closed even if exceptions occur during execution.
| synchronized (this) { | ||
| if (fastOpenRead != null && Objects.equals(config, fastOpenConfig)) { | ||
| Projection projection = (Projection) fastOpenRead.project(); | ||
| fastOpenRead = null; | ||
| if (projection instanceof ApiFuture) { | ||
| ApiFuture apiFuture = (ApiFuture) projection; | ||
| return (Projection) StorageException.coalesceAsync(apiFuture); | ||
| } | ||
| return projection; | ||
| } | ||
| } |
There was a problem hiding this comment.
There is logic duplication for handling ApiFuture and StorageException.coalesceAsync between the fast-open path and the fallback path. Additionally, if fastOpenRead is present but the config does not match, the pre-opened stream is currently ignored but remains held in the fastOpenRead field. This could lead to a resource leak if the session is used for multiple projections or if the stream is not otherwise closed. Consider refactoring to centralize the future handling and ensuring that fastOpenRead is cleared if it cannot be used for the requested projection.
References
- Resource management must be exception-safe to prevent leaks; ensure that all opened resources, such as streams, are properly closed or cleared if they cannot be used.
No description provided.