Open qinghui-xu opened 6 months ago
Given how the Java SDK uses stubs for type safety and how the Java SDK cannot use Java's native concurrent primitives I don't think this proposal is feasible as describe.
On workflow side we can not code it fluently using CompletionStage's compose APIs to combine activities in order (we have to use Temporal's Async static APIs for the purpose)
The Java SDK cannot support Java's native concurrent primitives like CompletableFuture
or CompletionStage
in a workflow since they are not suitable for our execution requirements, specifically around determinism so you must always use Temporals Async
code.
If an activity is invoked in a synchronous or asynchronous manner is determined by the caller (workflow), not the activity so a Future
should not be in the return type of the activity less you end up with a Promise
of a Future
of the actual response (which would be invalid anyway since Futures
cannot be used in workflow code).
This means an activity execution thread has to block and wait for the completion of a nonblocking / async task, which is not very efficient.
So if this is a concern the Java SDK support async activities without blocking a thread using https://javadoc.io/static/io.temporal/temporal-sdk/1.23.2/io/temporal/activity/ActivityExecutionContext.html#doNotCompleteOnReturn() you can see an example https://github.com/temporalio/sdk-java/blob/ed211fa611112288b576a2c979be9284e17fec89/temporal-sdk/src/test/java/io/temporal/workflow/activityTests/AsyncActivityCompleteWithErrorTest.java#L39
We also plan to support virtual threads in the near future that would also make this easier.
Hello Quinn, Thanks for your comments. Based on what you said, the main issue with my proposition is around the concurrent primitives for which Temporal SDK has to use its own. Thus I adapt my proposition a little bit to address your concerns with the following:
AsyncActivityMethod
should return java.util.concurrent.CompletableFuture
io.temporal.workflow.CompletablePromise
as an requirement (users have to convert their concurrent primitives such as Java CompletableFuture
or Scala Future
etc in their code).java.util.concurrent.CompletableFuture
io.temporal.workflow.CompletablePromise
but annotated with ActivityMethod
will continue to behave current way which is launching some async task and complete the activity without waiting for it (activity is just to launch something and does not care about the result), or this simply should be forbidden by raising exceptions when registering activities.AsyncActivityMethod
activity is considered done only when the returned io.temporal.workflow.CompletablePromise
is completed. This could be implemented in a similar way as what ManualActivityCompletionClient
does currently, but avoids boilerplates in user code.AsyncActivityMethod
will now need to handle the boilerplates for concurrent execution management in the same way as what Async.function
does todayThough what you said sounds good sense to me, there's still something I don't fully understand yet.
The Java SDK cannot support Java's native concurrent primitives like
CompletableFuture
orCompletionStage
in a workflow since they are not suitable for our execution requirements, specifically around determinism so you must always use TemporalsAsync
code.
IIUC (proposition is based on my understanding), the main issue is from the workflow side as composing CompletableFuture
will probably end up executing activity stubs in any thread which is not a WorkflowThread
, such that Temporal lost track of the "workflow tasks". Is my understanding correct or there's something I don't see through?
So if this is a concern the Java SDK support async activities without blocking a thread using https://javadoc.io/static/io.temporal/temporal-sdk/1.23.2/io/temporal/activity/ActivityExecutionContext.html#doNotCompleteOnReturn() you can see an example
Thanks for this example, and I pick some ideas from it as well to adapt my proposition. The issue with the current mechanism is that it's somehow intrusive to user code, and activity developers have to think about doing it. In fact this mechanism is documented, I read it quickly when I saw it for the first time, and I forgot about it quickly because I did not fully grasp the reason why it is proposed.
Is your feature request related to a problem? Please describe. In activity implementations, we often deal with Java async APIs (for example, the new
java.net.http
api in Java 11). And such APIs usually return aFuture
orCompletionStage
(less often), or even betterCompletableFuture
. Currently Activity invocation is not aware of these types and does not wait until the completion of them, which means we have to block and wait in client activity implementations (aka.ActivityMethod
). This is not very convenient:Future#get
in anActivityMethod
, and this will cause a bug such that temporal considers an activity as done while it is not. (This is more likely to happen if the activity ends up in calling some service that returns aCompletableFuture<Void>
as we usually don't expect to deal with the result specifically.)CompletionStage
's compose APIs to combine activities in order (we have to use Temporal'sAsync
static APIs for the purpose)Describe the solution you'd like To keep consistency with current behaviour and also make it explicit about (enabling) support of the java
CompletableFuture
, I propose to provideAsyncActivityMethod
annotation to be used on methods that are returning ajava.util.concurrent.CompletableFuture
and we expect the result to be completed before activity is done. More precisely:AsyncActivityMethod
should returnjava.util.concurrent.CompletableFuture
as an requirement.java.util.concurrent.CompletableFuture
but annotated withActivityMethod
will continue to behave current way which is launching some async task and complete the activity without waiting for it (activity is just to launch something and does not care about the result).AsyncActivityMethod
activity is considered done only when theCompletableFuture
is completed.Describe alternatives you've considered I am open to other propositions.
Additional context