Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async Activity support for Java async API #2049

Open
qinghui-xu opened this issue May 2, 2024 · 2 comments
Open

Async Activity support for Java async API #2049

qinghui-xu opened this issue May 2, 2024 · 2 comments
Labels
enhancement User experience

Comments

@qinghui-xu
Copy link

qinghui-xu commented May 2, 2024

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 a Future or CompletionStage (less often), or even better CompletableFuture. 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:

  • Developers may forget to call on Future#get in an ActivityMethod, 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 a CompletableFuture<Void> as we usually don't expect to deal with the result specifically.)
  • 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)
  • This means an activity execution thread has to block and wait for the completion of a nonblocking / async task, which is not very efficient.

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 provide AsyncActivityMethod annotation to be used on methods that are returning a java.util.concurrent.CompletableFuture and we expect the result to be completed before activity is done. More precisely:

  • A method annotated with AsyncActivityMethod should return java.util.concurrent.CompletableFuture as an requirement.
  • A mtheod returning java.util.concurrent.CompletableFuture 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).
  • AsyncActivityMethod activity is considered done only when the CompletableFuture is completed.

Describe alternatives you've considered
I am open to other propositions.

Additional context

@qinghui-xu qinghui-xu added the enhancement User experience label May 2, 2024
@Quinn-With-Two-Ns
Copy link
Contributor

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

We also plan to support virtual threads in the near future that would also make this easier.

@qinghui-xu
Copy link
Author

qinghui-xu commented May 3, 2024

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:

  • A method annotated with 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).
  • A mtheod returning 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.
  • [Activity side]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.
  • [Workflow side] Activitiy stubbing for AsyncActivityMethod will now need to handle the boilerplates for concurrent execution management in the same way as what Async.function does today

Though 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 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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement User experience
Projects
None yet
Development

No branches or pull requests

2 participants