gemini-code-assist[bot] commented on code in PR #38417:
URL: https://github.com/apache/beam/pull/38417#discussion_r3209930751
##########
sdks/java/io/rrio/src/main/java/org/apache/beam/io/requestresponse/Call.java:
##########
@@ -270,34 +270,35 @@ public void teardown() throws UserCodeExecutionException {
Sleeper sleeper = configuration.getSleeperSupplier().get();
backoffIfNeeded(backOff, sleeper);
-
- if (!configuration.getShouldRepeat()) {
- incIfPresent(teardownCounter);
- setupTeardown.teardown();
- return;
- }
-
- Repeater<Void, Void> repeater =
- Repeater.<Void, Void>builder()
- .setBackOff(backOff)
- .setSleeper(sleeper)
- .setThrowableFunction(
- ignored -> {
- incIfPresent(teardownCounter);
- setupTeardown.teardown();
- return null;
- })
- .build()
- .withBackoffCounter(backoffCounter)
- .withSleeperCounter(sleeperCounter);
-
- repeater.apply(null);
-
- checkStateNotNull(executor).shutdown();
try {
- boolean ignored = executor.awaitTermination(3L, TimeUnit.SECONDS);
- } catch (InterruptedException ignored) {
- // Ignore the interrupt during teardown.
+ if (!configuration.getShouldRepeat()) {
+ incIfPresent(teardownCounter);
+ setupTeardown.teardown();
+ return;
+ }
+
+ Repeater<Void, Void> repeater =
+ Repeater.<Void, Void>builder()
+ .setBackOff(backOff)
+ .setSleeper(sleeper)
+ .setThrowableFunction(
+ ignored -> {
+ incIfPresent(teardownCounter);
+ setupTeardown.teardown();
+ return null;
+ })
+ .build()
+ .withBackoffCounter(backoffCounter)
+ .withSleeperCounter(sleeperCounter);
+
+ repeater.apply(null);
+ } finally {
+ checkStateNotNull(executor).shutdown();
+ try {
+ boolean ignored = executor.awaitTermination(3L, TimeUnit.SECONDS);
+ } catch (InterruptedException ignored) {
+ // Ignore the interrupt during teardown.
+ }
Review Comment:
Using `checkStateNotNull(executor)` in a `finally` block is risky. If
`executor` is null (for example, if `setup()` failed before the executor was
initialized), this will throw an `IllegalStateException`. In Java, an exception
thrown in a `finally` block will suppress any exception thrown from the `try`
block, making debugging difficult. It is safer to use a null check here to
ensure `teardown()` is idempotent and doesn't mask other errors. Additionally,
the result of `awaitTermination` can be called without assigning it to a
variable, avoiding a potential name conflict with the catch parameter.
```java
if (executor != null) {
executor.shutdown();
try {
executor.awaitTermination(3L, TimeUnit.SECONDS);
} catch (InterruptedException ignored) {
// Ignore the interrupt during teardown.
}
}
```
--
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]
