Firestore.runTransaction should support/recognize asynchronous updateFunction result

[pavelkryl]

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.

[sduskis]

Your first resolution doesn't seem doable. If callbackValue is a T, how can it be cast to an ApiFuture<T>? That seems like an infinite recursion with Generics, which blows my mind.

I think that this needs more design work before this is actionable.

[pavelkryl]

You are right. The suggested code does not work. Moreover it's not a clean approach anyway.

The other suggestions are however still valid, I think.

[ajaaym]

@pavelkryl

This implies, that that whole body of the update function is executed in a blocking way, thus wasting precious resources.

What exactly do you mean by this? This is async from the perspective of calling thread as well as the internal thread. This is getting wrapped in to async function internally see here. does your update function depend on some other async function? If thats the case then probably you should run this whole transaction after your async function is done. would that work?

[pavelkryl]

@ajaaym

I am discussing:

callbackResult.set(transactionCallback.updateCallback(transaction));

You are describing what happens after the updateCallback() is invoked: we have the result of type T and an asynchronous result processing happens. However, what I mean is how you get the return value of udpateCallback(tx) - you retrieve this synchronously and it must be of type T, not ApiFuture, unfortunately:

public interface Function<T> {

    T updateCallback(Transaction transaction) throws Exception;

}

Let's pretend that in the updateCallback(tx), I am doing some RESTful I/O. I would like to provide the result asynchronously - because I am using netty for example. But, instead I have to block the thread executing updateCallback(tx), because I need to provide the result of type T synchronously. This is the wasting of precious resources I am talking about.

Is that now clearer?

[ajaaym]

@pavelkryl Can you do Rest operation first and then call runTransaction? runTransaction is the actual public interface, which is async.

[pavelkryl]

@ajaaym Yes I can. But this debate is a conceptual one. Doing a RESTful call is just one example of hundreds of usecases where asynchronous interaction would suit better than a synchronous one. Similarly I could ask: could you pre-compute the value T and pass it directly as value without the necessity to call updateCallback()? Yes you can. But having a possibility to pass a function which computes the value is better than having the value directly.

Moreover in the transaction callback you will often call firestore API to read a document - Transaction.get() - that returns also ApiFuture right? So there is a conceptual gap: I have an asynchronous API calls to get documents from firestore, but I cannot utilize this asynchronicity, because the updateCallback() requires directly value of type T and not ApiFuture<T>.

Please distinguish between runTransaction() API call (which is async) and updateCallback() (which does not allow me to provide a value asynchronously).

Suggested Solution

Presuming that you support async callback:

public interface AsyncFunction<T> {

    ApiFuture<T> updateCallbackAsync(Transaction transaction) throws Exception;

}

The implementation is that simple:

            userCallbackExecutor.execute(
                new Runnable() {
                  @Override
                  public void run() {

                    try {
                      ApiFuture<T> callbackFuture = transactionCallback.updateCallbackAsync(transaction);
                      ApiFutures.addCallback(callbackFuture, new ApiFutureCallback<T>() {
                          @Override
                          public void onFailure(Throwable throwable) {
                            callbackResult.setException(throwable);
                          }

                          @Override
                          public void onSuccess(T t) {
                            callbackResult.set(t);
                          }
                      });
                    } catch (Throwable t) {
                      callbackResult.setException(t);
                    }
                  }
                });

This does not seem to me that complicated, but the benefit it brings is huge.

[pavelkryl]

Hey, could someone answer me - or oppose the idea?

[pavelkryl]

Hey, why isn't anybody replying to my ingenious suggestion? :) This is 30 line improvement with huge implications - supporting fully asynchronous execution.