codelipenghui commented on code in PR #25296:
URL: https://github.com/apache/pulsar/pull/25296#discussion_r2902920304
##########
tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlobStoreBackedReadHandleImplV2.java:
##########
@@ -132,17 +132,34 @@ public CompletableFuture<Void> closeAsync() {
CompletableFuture<Void> promise = closeFuture.get();
executor.execute(() -> {
- try {
- for (OffloadIndexBlockV2 indexBlock : indices) {
+ IOException first = null;
+ for (OffloadIndexBlockV2 indexBlock : indices) {
Review Comment:
The individual `try/catch` blocks only catch `IOException`. If any `close()`
throws a `RuntimeException`, it propagates out of the lambda, gets swallowed by
the `ExecutorService`, and `promise` is never completed — callers hang forever
on `closeAsync().get()`. Consider wrapping the entire lambda body in `try { ...
} catch (Throwable t) { state = State.Closed; promise.completeExceptionally(t);
}`.
##########
tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlobStoreBackedReadHandleImplV2.java:
##########
@@ -132,17 +132,34 @@ public CompletableFuture<Void> closeAsync() {
CompletableFuture<Void> promise = closeFuture.get();
executor.execute(() -> {
- try {
- for (OffloadIndexBlockV2 indexBlock : indices) {
+ IOException first = null;
+ for (OffloadIndexBlockV2 indexBlock : indices) {
+ try {
indexBlock.close();
+ } catch (IOException e) {
+ if (first == null) {
+ first = e;
+ } else {
+ first.addSuppressed(e);
+ }
}
- for (DataInputStream dataStream : dataStreams) {
+ }
+ for (DataInputStream dataStream : dataStreams) {
+ try {
dataStream.close();
+ } catch (IOException e) {
+ if (first == null) {
+ first = e;
+ } else {
+ first.addSuppressed(e);
+ }
}
+ }
+ if (first != null) {
+ promise.completeExceptionally(e);
+ } else {
state = State.Closed;
promise.complete(null);
Review Comment:
`state` should be set to `Closed` unconditionally (move it before the
`if/else`), not only on the success path. Once all resources have had `close()`
attempted, the handle is effectively closed. Leaving it as `Opened` on failure
means `readAsync()` won't reject calls, but the underlying resources are
already closed, causing confusing secondary errors. Also, `closeFuture` is
already set via `compareAndSet`, so retrying `closeAsync()` returns the same
failed future — the handle becomes permanently stuck.
##########
tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlobStoreBackedReadHandleImplV2.java:
##########
@@ -132,17 +132,34 @@ public CompletableFuture<Void> closeAsync() {
CompletableFuture<Void> promise = closeFuture.get();
executor.execute(() -> {
- try {
- for (OffloadIndexBlockV2 indexBlock : indices) {
+ IOException first = null;
+ for (OffloadIndexBlockV2 indexBlock : indices) {
+ try {
indexBlock.close();
+ } catch (IOException e) {
+ if (first == null) {
+ first = e;
+ } else {
+ first.addSuppressed(e);
+ }
}
- for (DataInputStream dataStream : dataStreams) {
+ }
+ for (DataInputStream dataStream : dataStreams) {
+ try {
dataStream.close();
+ } catch (IOException e) {
+ if (first == null) {
+ first = e;
+ } else {
+ first.addSuppressed(e);
+ }
}
+ }
+ if (first != null) {
+ promise.completeExceptionally(e);
+ } else {
Review Comment:
Bug: `e` is not in scope here — it only exists inside the `catch` blocks.
Should be `first`. This will not compile.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
