Firestore.runTransaction should support/recognize asynchronous updateFunction result #43
Description
Thanks for creating and managing Firestore. I have one suggestion for improvement though.
Currently I consider Firestore admin API to be just partially asynchronous. Whenever there is a call which returns ApiFuture, I consider it OK, although far from ideal comparing to RxJava/reactor Single/Observable/Mono/Flux. I can do conversion from ApiFuture to Mono quite easily by myself.
Problem
The Firestore.runTransaction() has a problem: result of the runTransaction is OK (it's ApiFuture). The transaction update function, however, must provide a result synchronously:
public interface Function<T> {
T updateCallback(Transaction transaction) throws Exception;
}
When I track down to the point where the function is executed, I observe:
private SettableApiFuture<T> invokeUserCallback() {
// Execute the user callback on the provided executor.
final SettableApiFuture<T> callbackResult = SettableApiFuture.create();
userCallbackExecutor.execute(
new Runnable() {
@Override
public void run() {
try {
callbackResult.set(transactionCallback.updateCallback(transaction));
} catch (Throwable t) {
callbackResult.setException(t);
}
}
});
return callbackResult;
}
This implies, that that whole body of the update function is executed in a blocking way, thus wasting precious resources.
Resolution
My suggestion for improvement is instead of calling callbackResult.set(transactionCallback.updateCallback(transaction)), it would be better to distinguish asynchronous result and do the invocation specifically:
userCallbackExecutor.execute(
new Runnable() {
@Override
public void run() {
try {
T callbackValue = transactionCallback.updateCallback(transaction);
if (callbackValue instanceof ApiFuture) {
ApiFutures.addCallback((ApiFuture<T>) callbackValue, new ApiFutureCallback<T>() {
@Override
public void onFailure(Throwable throwable) {
callbackResult.setException(throwable);
}
@Override
public void onSuccess(T t) {
callbackResult.set(t);
}
});
} else {
callbackResult.set(callbackValue);
}
} catch (Throwable t) {
callbackResult.setException(t);
}
}
});
With this implementation, I could implement body of the update function in a fully reactive way and convert the result to ApiFuture (SettableApiFuture).
Alternatives
A cleaner solution - describing the contract more clearly - would be to have separate interface for asynchronous update function:
public interface AsyncFunction<T> {
ApiFuture<T> updateCallbackAsync(Transaction transaction) throws Exception;
}
And then specific method for executing AsyncFunction instead of Function. Actually anything which somehow allows me to do asynchronous transaction execution would offer me the required flexibility/efficiency.
Another alternative would be to provide fully reactive API (returning Mono/Single), but I understand that you do not want dependency on particular vendor (RxJava, Spring reactor). Maybe, however, you could consider switching to reactive streams Publisher, which is becoming the common shared API/standard for reactive programming in Java.