Properly support CompletionStage as a return type in caching extension

[geoand]

Fixes: #23816

[gwenneg]

Thank you @geoand!

I'm not done reviewing the tests but first I have a few comments about the runtime code.

[geoand]

Thanks for the feedback. I'll have a look tomorrow as I am taking the day off today

…

On Wed, Apr 13, 2022, 12:30 Gwenneg Lepage ***@***.***> wrote: ***@***.**** commented on this pull request. Thank you @geoand <https://github.com/geoand>! I'm not done reviewing the tests but first I have a few comments about the runtime code. ------------------------------ In extensions/cache/runtime/src/main/java/io/quarkus/cache/runtime/CacheInterceptor.java <#24894 (comment)>: > @@ -27,6 +28,7 @@ public abstract class CacheInterceptor { private static final Logger LOGGER = Logger.getLogger(CacheInterceptor.class); private static final String PERFORMANCE_WARN_MSG = "Cache key resolution based on reflection calls. Please create a GitHub issue in the Quarkus repository, the maintainers might be able to improve your application performance."; + protected static final String UNHANDLED_ASYNC_RETURN_TYPE_MSG = "Unhandled async type"; ⬇️ Suggested change - protected static final String UNHANDLED_ASYNC_RETURN_TYPE_MSG = "Unhandled async type"; + protected static final String UNHANDLED_ASYNC_RETURN_TYPE_MSG = "Unhandled async return type"; ------------------------------ In extensions/cache/runtime/src/main/java/io/quarkus/cache/runtime/CacheInterceptor.java <#24894 (comment)>: > + if (Uni.class.isAssignableFrom(returnType)) { + return ReturnType.Uni; + } + if (CompletionStage.class.isAssignableFrom(returnType)) { + return ReturnType.CompletionStage; + } + return ReturnType.NonAsync; + } + + protected Uni<?> asyncInvocationResultToUni(Object invocationResult, ReturnType returnType) { + if (returnType == ReturnType.Uni) { + return (Uni<?>) invocationResult; + } else if (returnType == ReturnType.CompletionStage) { + return Uni.createFrom().completionStage((CompletionStage<?>) invocationResult); + } else { + throw new IllegalStateException(UNHANDLED_ASYNC_RETURN_TYPE_MSG); This could throw a CacheException instead of the ISE. ------------------------------ In extensions/cache/runtime/src/main/java/io/quarkus/cache/runtime/CacheInterceptor.java <#24894 (comment)>: > + return (Uni<?>) invocationResult; + } else if (returnType == ReturnType.CompletionStage) { + return Uni.createFrom().completionStage((CompletionStage<?>) invocationResult); + } else { + throw new IllegalStateException(UNHANDLED_ASYNC_RETURN_TYPE_MSG); + } + } + + protected Object createAsyncResult(Uni<Object> cacheValue, ReturnType returnType) { + if (returnType == ReturnType.Uni) { + return cacheValue; + } + if (returnType == ReturnType.CompletionStage) { + return cacheValue.subscribeAsCompletionStage(); + } + throw new IllegalStateException(UNHANDLED_ASYNC_RETURN_TYPE_MSG); This could throw a CacheException instead of the ISE. ------------------------------ In extensions/cache/runtime/src/main/java/io/quarkus/cache/runtime/CacheInterceptor.java <#24894 (comment)>: > - return Uni.class.isAssignableFrom(invocationContext.getMethod().getReturnType()); + protected static ReturnType determineReturnType(Class<?> returnType) { + if (Uni.class.isAssignableFrom(returnType)) { + return ReturnType.Uni; + } + if (CompletionStage.class.isAssignableFrom(returnType)) { + return ReturnType.CompletionStage; + } + return ReturnType.NonAsync; + } + + protected Uni<?> asyncInvocationResultToUni(Object invocationResult, ReturnType returnType) { + if (returnType == ReturnType.Uni) { + return (Uni<?>) invocationResult; + } else if (returnType == ReturnType.CompletionStage) { + return Uni.createFrom().completionStage((CompletionStage<?>) invocationResult); I'm not sure @CacheResult#lockTimeout will work if the Uni is created from the CompletionStage here. Delaying the CompletionStage computation (with a Supplier<CompletionStage>) until the Uni is resolved may be necessary: ⬇️ Suggested change - return Uni.createFrom().completionStage((CompletionStage<?>) invocationResult); + return Uni.createFrom().completionStage(() -> (CompletionStage<?>) invocationContext.proceed()); (the real code would need a proper Supplier and not that lambda 😄) — Reply to this email directly, view it on GitHub <#24894 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBMDP6DOGRA44UTO23RZULVE2H47ANCNFSM5TH3REAA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[geoand]

Comments addressed

[geoand]

This stuff will likely need to be rewritten for #24894, but it's best to separate the two as this one addresses a clear bug, while the Kotlin corroutine support is a feature request

[gwenneg]

I should be able to finish this review later today, sorry for the delay 😃

[geoand]

Not a problem at all :)

[gwenneg]

Sorry for the delay 😃

I noticed something wrong with CacheResult#lockTimeout and Uni while reviewing this PR, so I spent some time confirming that there is indeed a bug in that area. CompletionStage is affected in a similar way. I'll create an issue about it or a PR to fix it depending on how much time I have this week.

[gwenneg]

I hope I didn't break a test with the variables names changes 😄

[geoand]

Thanks a lot for the review, suggestions applied and commits squashed :)

[gwenneg]

LGTM