temporalio / sdk-java

Temporal Java SDK
https://temporal.io
Apache License 2.0
212 stars 143 forks source link

Support Scala references to workflow methods #1489

Open scoplin opened 1 year ago

scoplin commented 1 year ago

Is your feature request related to a problem? Please describe.

If the Async.function (or similar) methods is called in an attempt to dispatch a child workflow from a Scala class, it fails to properly detect the attempt to call a workflow method on a temporal stub, and instead falls back to submitting the function to a new workflow thread in AsyncInternal. As a result, we saw excessive threads being created and consumed in workers, eventually resulting in RejectedExectionException being thrown from the underlying executor.

For reference, the structure of the Scala code to demonstrate the problem is:

val stub = Workflow.newChildWorkflowStub(classOf[MyWorkflow], workflowOptions)
val result = Async.function(stub.performMyWorkflow, workflowInput)  // Problem occurs here

Describe the solution you'd like

The logic in MethodReferenceDisassembler.isAsync(Object) should be modified to account for Scala method references. When I investigated this with a debugger, I saw that this line in isAsyncJava appeared to be where a modification could be made:

&& lambda.getImplMethodKind() == MethodHandleInfo.REF_invokeInterface);

With a Scala method reference, the implementation method kind is equal to MethodHandleInfo.REF_invokeStatic. In my particular case, both Java and Scala are giving me a target reference of type AsyncInternal.AsyncMarker. I'm hoping that a fix is as simple as allowing both types of method implementation kinds may be all that's required here.

This is substantially less than the request for full Scala support covered by #1007. That is obviously a lot more work.

Describe alternatives you've considered

I was able to work around this issue by creating a small java shim class to call Async.function using a Java method reference.

Additional context

My local debugging was done against Scala 2.12. I am not sure to what degree these behaviors differ across Scala versions.

It appears to me that the vitaliihonta/ztemporal project may also run afoul of this introspection problem based upon how it uses Async in its ZPromise.

ploddi commented 1 year ago

@scoplin can you share you Java workaround?

Spikhalskiy commented 1 year ago

I solved the same problem for Kotlin here: KotlinMethodReferenceDisassemblyService

There is a handy abstraction that can be implemented for Scala (if at all possible to do) to support their method references as natively as Java ones: MethodReferenceDisassemblyService

There are also tests in KotlinAsyncChildWorkflowTest, KotlinAsyncCompanionFunctionTest, KotlinAsyncLambdaTest that may give a framework for the same tests for Scala.

If you are interested in researching and disassembling Scala's method references, we would be open to getting a temporal-scala module that will bring the method reference disassembly service for Scala.

ploddi commented 1 year ago

@Spikhalskiy I believe that Scala can use same DisassemblyService as Java, with the only difference that Scala compiler generates different implementation method kinds for lambdas, so actual stub checking method should be adjusted for Scala specific method references.

ploddi commented 1 year ago

Do we even need this check?

&& lambda.getImplMethodKind() == MethodHandleInfo.REF_invokeInterface);

Because without it Scala method references will work.

scoplin commented 1 year ago

@ploddi My workaround was method-specific. I simply took this line:

val result = Async.function(stub.performMyWorkflow, workflowInput)  // Problem occurs here

And wrote/compiled it in java instead as a trivial static method. Then I called that wrapper method from scala.