dcapwell commented on code in PR #17:
URL: https://github.com/apache/cassandra-accord/pull/17#discussion_r1060973217
##########
accord-core/src/main/java/accord/utils/async/AsyncCallbacks.java:
##########
@@ -0,0 +1,45 @@
+package accord.utils.async;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.concurrent.Executor;
+import java.util.function.BiConsumer;
+
+public class AsyncCallbacks
+{
+ private static final Logger logger =
LoggerFactory.getLogger(AsyncCallbacks.class);
+
+ private static final BiConsumer<Object, Throwable> NOOP = (unused,
failure) -> {
+ if (failure != null)
+ logger.error("Exception received by noop callback", failure);
+ };
+
+ public static <T> BiConsumer<? super T, Throwable> noop()
+ {
+ return NOOP;
+ }
+
+ public static <T> BiConsumer<? super T, Throwable> inExecutor(BiConsumer<?
super T, Throwable> callback, Executor executor)
+ {
+ return (result, throwable) -> {
+ try
+ {
+ executor.execute(() -> callback.accept(result, throwable));
+ }
+ catch (Throwable t)
+ {
+ callback.accept(null, t);
+ }
+ };
+ }
+
+
+ public static <T> BiConsumer<? super T, Throwable> inExecutor(Runnable
runnable, Executor executor)
+ {
+ return (result, throwable) -> {
+ if (throwable == null) executor.execute(runnable);
+ else throw new RuntimeException(throwable);
+ };
+ }
+}
Review Comment:
> should we just not submit to the executor if throwable is non-null?
I spent some time triggering failures at different locations to see that we
are currently not consistent... so we should flesh out the semantics and push
this to the type system...
Here are the cases I tested
```
AsyncChains.ofCallable(ignore -> {
throw new RejectedExecutionException();
}, () -> 42)
.map(i -> i + 1)
.begin((success, failure) -> {
if (failure == null)
throw new IllegalStateException("Should see
failure");
});
```
`begin` throws, so caller needs to handle.
```
AsyncChains.ofCallable(ignore -> {
throw new RejectedExecutionException();
}, () -> 42)
.map(i -> i + 1)
.beginAsResult()
.addCallback((success, failure) -> {
if (failure == null)
throw new IllegalStateException("Expected to fail");
});
```
`beginAsResult ` throws, so caller needs to handle.
```
AsyncChains.<Integer>ofCallable(fn -> fn.run(), () -> {
throw new RuntimeException("Unchecked");
}).map(i -> i + 1).map(i -> i + 1)
.begin((success, failure) -> {
if (failure == null)
throw new IllegalStateException("Should see
failure");
});
```
`begin` sees the exception rather than throws it, so caller doesn't see an
exception
```
AsyncChains.<Integer>ofCallable(fn -> fn.run(), () -> {
throw new RuntimeException("Unchecked");
}).map(i -> i + 1).map(i -> i + 1)
.begin(() -> {
});
```
`begin` throws, so caller needs to handle.
```
AsyncChains.ofCallable(fn -> fn.run(), () -> 42
).map(i -> i + 1)
.map(ignore -> {
throw new RuntimeException("Unchecked");
})
.begin((success, failure) -> {
if (failure == null)
throw new IllegalStateException("Should see
failure");
});
```
`begin` does not fail here, the error is seen by the callback
```
AsyncChains.ofCallable(fn -> fn.run(), () -> 42).
map(i -> i + 1)
.map(ignore -> {
throw new RuntimeException("Unchecked");
}).begin(() -> {
});
```
`begin` throws, so caller needs to handle.
Small changes have big impacts to the callers of `begin`, which may not know
about what happened to the other parts of the chain...
Personally I find this behavior brittle, and would propose the following
1) `begin(Runnable)` is removed or replaced with a `begin(Runnable,
Consumer<Throwable>)`. This is only used by tests, so safest to just remove.
2) we catch at all places and send the exception down the chain, `begin`
should never throw
Main reason for #2 is that the call to `begin` is expected to handle
exceptions async, so by having it also throw, we make the usage harder to do
correctly... by centralizing the exceptions we avoid future issues.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
