Add API to make it easier to write method interceptors and support Kotlin suspend functions

[dstepanov]

Added util classes to work with Kotlin's suspend functions in MethodInterceptors and implemented to use it with @Retryable.

It should be possible to implement other interceptors like micronaut-projects/micronaut-cache#183.

Personally I'm not a Kotlin user just found it interesting.

[graemerocher]

Nice work! Definitely a 2.1 feature this

[dstepanov]

I think there should be a better way to do interception instead of the current code that is almost the same everywhere: "check if publisher", "check if future" and now this will require adding "check if kotlin suspend".

[graemerocher]

@dstepanov yes we need some generalized abstraction for handling all these cases I agree

[dstepanov]

Extracted a helper class, looks much better now.

[graemerocher]

Seem to be some checkstyle violations

[graemerocher]

The only doubt I have about this is I am unsure if it is going into micronaut-aop that we should couple it to RxJava 2

In general I think we need to move rxjava 2 to a module like the other frameworks and add a dependency on the modules that use it to RxJava 2 to make sure we have a road to move to rxjava 3 in the future

[dstepanov]

I have removed RxJava 2 dependency, only one use case with publishers/futures that is not changed is Http client.

[dstepanov]

@graemerocher I have made some refactoring to introduce reactive introduce call API, the core should have all interceptors refactored to use new API. Let me know what do you think.

[graemerocher]

@dstepanov this is really awesome work and will definitely help avoid duplication! There are a few things I am unsure about:

• I am not sure if reactive() is the correct name here since the helper handles more than just reactive but also completion stage and synchronous. Maybe interceptBuilder() or something would be more appropriate. I would also rename ReactiveMethodInvocationInterceptor to InterceptBuilder
• I am confused by why retry handling exists for completion stage in this helper https://github.com/micronaut-projects/micronaut-core/pull/3921/files#diff-35c23af827950f588d03002d3b502d97R62 it seems like something that should not be part of the core API?

[dstepanov]

@graemerocher To retry the method call the interceptor needs to be supplied context.proceed(interceptor), not sure how to represent it in the API of the reactive interceptor. Maybe to have just one Supplier which would call context.proceed() for the first invocation and context.proceed(interceptor) next. WDYT?

[graemerocher]

ah ok that confused me, maybe it should be called restart or reset instead as you are restarting from the beginning of the interceptor chain?

[dstepanov]

Naming is not the best, for the users of the API it might not be very clear what is the interceptor chain etc. that's why retry is most clear but it does hide some impl. details.

[dstepanov]

@graemerocher How would you name InterceptCompletionStageSupplierWithRetry?

[graemerocher]

@dstepanov maybe InterceptCompletionStageRestartSupplier?

[dstepanov]

Refactored ValidatingInterceptor to use the builder

[graemerocher]

I think we should probably leave it like it is for now

[graemerocher]

@dstepanov one final change. I think we should not include this as a method on MethodInvocationContext and instead have a static factory method on the interface. I can image wanting to create subclasses of this to support other reactive frameworks and maybe the Flow API in Java 9

[dstepanov]

@graemerocher something like static InterceptBuilder<T, R> of(MethodInvocationContext<T, R> context) on InterceptBuilder?

[graemerocher]

yes something like that

[dstepanov]

@graemerocher Renamed

[dstepanov]

I was thinking maybe it would be better to replace null checks with instanceof, impl. would need to pass an instance that implements 2-3 interfaces.

     return InterceptHelper.intercept(new ReactiveIntercept() {
            @Override
            public CompletionStage<Object> intercept(Argument<Object> valueType, Supplier<CompletionStage<Object>> resultSupplier, Supplier<CompletionStage<Object>> retryResultSupplier) {
                ...
            }

            @Override
            public Publisher<Object> intercept(Argument<Object> valueType, Supplier<Publisher<Object>> publisherSupplier) {
                ...
            }

            @Override
            public Object intercept() {
                ...
            }
        }, this);
    }
    
    private abstract class ReactiveRetryIntercept implements InterceptPublisherSupplier, InterceptCompletionStageRestartSupplier, InterceptSynchronous {
    }

The API is a bit strange because Java doesn't allow it to implement multiple interfaces for anonymous classes, but the performance should be better.

Also I would add MethodInvocationContext<Object, Object> context to every new intercept interface to look it more like MethodInterceptor<Object, Object>.

WDYT?

[dstepanov]

@graemerocher Should I try to refactor it?

[graemerocher]

@dstepanov I wonder if it is worth providing a worse API in the name of performance.

[dstepanov]

Probably not

[jameskleeh]

@dstepanov I'd like to get this merged in pretty soon so I can look into coroutine support for the client. Overall it looks to be a big improvement over what we have currently. I am also concerned about the performance implication and I'd like to propose an alternative.

What if instead of having a builder style, an object was returned InterceptedMethod.of(context) which would then have methods on it to return the type (an enum), among others. The calling code could then have a switch block to handle the different types with a fallback default of interceptedMethod.unsupported() or something of the sort. This wouldn't require the multiple lambdas to be created for every method call.

I haven't dug into this logic very deeply so perhaps this wouldn't work for some reason, but it seems like it wouldn't complicate the usage of the API very much.

[dstepanov]

@jameskleeh That's one of the ways to do it with the disadvantage of moving the switch outside to every interceptor and removing the strick check.

It would be possible to completely remove the builder:

switch(InterceptMethodUtils.invocationType(context)) { 
    case COMPLETION_STAGE:
        return InterceptMethodUtils.interceptCompletionStage(context, (..) -> {});
    case PUBLISHER:
         return ..
    case SYNCHRONOUS:
        return ..
}

This PR also adds coroutine support for the http client:

micronaut-core/test-suite-kotlin/src/test/kotlin/io/micronaut/docs/server/suspend/SuspendClient.kt

Lines 18 to 47 in 880b2f4

	import io.micronaut.http.HttpResponse
	import io.micronaut.http.MediaType
	import io.micronaut.http.annotation.Get
	import io.micronaut.http.client.annotation.Client
	@Client("/suspend")
	interface SuspendClient {
	@Get("/simple", consumes = [MediaType.TEXT_PLAIN])
	suspend fun simple(): String
	@Get("/simple", consumes = [MediaType.TEXT_PLAIN])
	suspend fun simpleIgnoreResult()
	@Get("/simple", consumes = [MediaType.TEXT_PLAIN])
	suspend fun simpleResponse(): HttpResponse<String>
	@Get("/simple", consumes = [MediaType.TEXT_PLAIN])
	suspend fun simpleResponseIgnoreResult(): HttpResponse<Any>
	@Get("/delayed", consumes = [MediaType.TEXT_PLAIN])
	suspend fun delayed(): String
	@Get("/illegal", consumes = [MediaType.ALL])
	suspend fun errorCall(): String
	@Get("/illegal", consumes = [MediaType.ALL])
	suspend fun errorCallResponse(): HttpResponse<String>
	}

[jameskleeh]

@dstepanov I see now that coroutines are handled as part of the completion stage handlers. Awesome!

@graemerocher Can you give your thoughts? Perhaps we should have an idea of the performance implications before moving to a less friendly API.

[graemerocher]

@jameskleeh only way to know the performance implications is benchmarking, however in general it is safe to assume a switch style would be much faster and more efficient in terms of memory consumption since there is less object creation to perform method dispatch.

[dstepanov]

Should I make some changes?

[jameskleeh]

@dstepanov You can, however I think the code you showed above wouldn't result in increased performance:

switch(InterceptMethodUtils.invocationType(context)) { 
    case COMPLETION_STAGE:
        return InterceptMethodUtils.interceptCompletionStage(context, (..) -> {}); //still using a lambda here

I think it would need to be more like

InterceptedMethod interceptedMethod = InterceptedMethod.of(context);
switch (interceptedMethod.returnType()) {
    case COMPLETION_STAGE:
        CompletableFuture f = toCompletableFuture(invokeFn(body, functionName, functionDefinition, valueType))
        return interceptedMethod.handleResult(f);
        break;
    default: //strict mode, up to the caller if the call this
        interceptedMethod.unsupported(); //throws exception

If that seems doable and you can get to it within the next week or so, that would be fantastic. If not, I will make sure it gets taken care of before 2.1

[dstepanov]

@jameskleeh @graemerocher Please review

[graemerocher]

That looks great to me. @jameskleeh ?

[jameskleeh]

@graemerocher Looks good to me. Really great contribution @dstepanov!

[dstepanov]

@jameskleeh Thanks! You had a good suggestion for the API