micronaut-core icon indicating copy to clipboard operation
micronaut-core copied to clipboard

There is no control for the event loop selection outside the controller method e.g. in TypedArgumentBinder

Open musketyr opened this issue 1 year ago • 11 comments

Expected Behavior

Either the code preceding the controller method execution such as authentication resolution or argument binding respect the event loop selection for the controller or every API such as TypedArgumentBinder supports reactive programming.

Actual Behaviour

The @ExcuteOn annotation only applies to the body of the controller methods. Anything outside the controller method such as argument binding using RequestArgumentSatisfier and TypedArgumentBinder always happens on the Netty event loop making fixing the new blocking operation exceptions very verbose and unfriendly.

Steps To Reproduce

Checkout the repository below with branch example-with-blocking-http-call-in-arugment-binder and run FactsControllerTest.

Environment Information

JDK 21

Example Application

https://github.com/musketyr/micronaut-blocking-calls-in-binders-issue/tree/example-with-blocking-http-call-in-arugment-binder

Version

4.4:0

musketyr avatar Apr 19 '24 06:04 musketyr

It is probably the answer you are looking for but I think the solution is not to do network calls in the TypeArgumentBinder. I think you should move that logic (network requests) to a filter or to a controller method.

sdelamo avatar Apr 19 '24 07:04 sdelamo

I appreciate your advice but this would mean rewriting hundreds of controller methods. and some of the code is out of our control. e..g fetching the token. we need to clean up the MN versions, we're still getting errors from JWKS verification even we are trying to offload to the blocking scheduler.

09:52:38.208 [default-nioEventLoopGroup-2-2] ERROR io.micronaut.http.server.RouteExecutor - Unexpected error occurred: Error instantiating bean of type  [io.micronaut.security.token.TokenAuthenticationFetcher]

Message: blockOptional() is blocking, which is not supported in thread default-nioEventLoopGroup-2-2
Path Taken: new SecurityFilter(Collection securityRules,Collection authenticationFetchers,SecurityConfiguration securityConfiguration) --> new SecurityFilter(Collection securityRules,[Collection authenticationFetchers],SecurityConfiguration securityConfiguration) --> new TokenAuthenticationFetcher([List tokenValidators],TokenResolver tokenResolver,ApplicationEventPublisher tokenValidatedEventPublisher,HttpHostResolver httpHostResolver,HttpLocaleResolver httpLocaleResolver)
io.micronaut.context.exceptions.BeanInstantiationException: Error instantiating bean of type  [io.micronaut.security.token.TokenAuthenticationFetcher]

Message: blockOptional() is blocking, which is not supported in thread default-nioEventLoopGroup-2-2
Path Taken: new SecurityFilter(Collection securityRules,Collection authenticationFetchers,SecurityConfiguration securityConfiguration) --> new SecurityFilter(Collection securityRules,[Collection authenticationFetchers],SecurityConfiguration securityConfiguration) --> new TokenAuthenticationFetcher([List tokenValidators],TokenResolver tokenResolver,ApplicationEventPublisher tokenValidatedEventPublisher,HttpHostResolver httpHostResolver,HttpLocaleResolver httpLocaleResolver)
	at io.micronaut.context.DefaultBeanContext.resolveByBeanFactory(DefaultBeanContext.java:2326) ~[micronaut-inject-4.4.3.jar:4.4.3]
	at io.micronaut.context.DefaultBeanContext.doCreateBean(DefaultBeanContext.java:2281) ~[micronaut-inject-4.4.3.jar:4.4.3]
	at io.micronaut.context.DefaultBeanContext.doCreateBean(DefaultBeanContext.java:2293) ~[micronaut-inject-4.4.3.jar:4.4.3]
	at io.micronaut.context.DefaultBeanContext.createRegistration(DefaultBeanContext.java:3095) ~[micronaut-inject-4.4.3.jar:4.4.3]
	at io.micronaut.context.SingletonScope.getOrCreate(SingletonScope.java:80) ~[micronaut-inject-4.4.3.jar:4.4.3]
	at io.micronaut.context.DefaultBeanContext.findOrCreateSingletonBeanRegistration(DefaultBeanContext.java:2997) ~[micronaut-inject-4.4.3.jar:4.4.3]
	at io.micronaut.context.DefaultBeanContext.resolveBeanRegistration(DefaultBeanContext.java:2958) ~[micronaut-inject-4.4.3.jar:4.4.3]
	at io.micronaut.context.DefaultBeanContext.resolveBeanRegistration(DefaultBeanContext.java:2932) ~[micronaut-inject-4.4.3.jar:4.4.3]
	at io.micronaut.context.DefaultBeanContext.addCandidateToList(DefaultBeanContext.java:3521) ~[micronaut-inject-4.4.3.jar:4.4.3]
	at io.micronaut.context.DefaultBeanContext.resolveBeanRegistrations(DefaultBeanContext.java:3476) ~[micronaut-inject-4.4.3.jar:4.4.3]
	at io.micronaut.context.DefaultBeanContext.getBeanRegistrations(DefaultBeanContext.java:3450) ~[micronaut-inject-4.4.3.jar:4.4.3]
...
	at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919) [netty-transport-4.1.108.Final.jar:4.1.108.Final]
	at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:166) [netty-transport-4.1.108.Final.jar:4.1.108.Final]
	at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:788) [netty-transport-4.1.108.Final.jar:4.1.108.Final]
	at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:724) [netty-transport-4.1.108.Final.jar:4.1.108.Final]
	at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:650) [netty-transport-4.1.108.Final.jar:4.1.108.Final]
	at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:562) [netty-transport-4.1.108.Final.jar:4.1.108.Final]
	at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997) [netty-common-4.1.108.Final.jar:4.1.108.Final]
	at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74) [netty-common-4.1.108.Final.jar:4.1.108.Final]
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) [netty-common-4.1.108.Final.jar:4.1.108.Final]
	at java.base/java.lang.Thread.run(Thread.java:840) [?:?]
Caused by: java.lang.IllegalStateException: blockOptional() is blocking, which is not supported in thread default-nioEventLoopGroup-2-2
	at reactor.core.publisher.BlockingOptionalMonoSubscriber.blockingGet(BlockingOptionalMonoSubscriber.java:108) ~[reactor-core-3.6.4.jar:3.6.4]
	at reactor.core.publisher.Mono.blockOptional(Mono.java:1831) ~[reactor-core-3.6.4.jar:3.6.4]
	at io.micronaut.security.token.jwt.signature.jwks.JwksSignature.loadJwkSet(JwksSignature.java:178) ~[micronaut-security-jwt-4.7.0.jar:4.7.0]
	at io.micronaut.security.token.jwt.signature.jwks.JwksSignature.computeJWKSet(JwksSignature.java:78) ~[micronaut-security-jwt-4.7.0.jar:4.7.0]
	at io.micronaut.security.token.jwt.signature.jwks.JwksSignature.getKeyIds(JwksSignature.java:112) ~[micronaut-security-jwt-4.7.0.jar:4.7.0]

musketyr avatar Apr 19 '24 08:04 musketyr

have you tried the executor service option described here: https://www.youtube.com/watch?v=W6iztOuulVU ?

sdelamo avatar Apr 19 '24 08:04 sdelamo

I would probably put your code in a filter which you can annotate to @ExecuteOn, populate a request attribute in the file and then keep the logic of the request argument simple fetching from request attribute.

sdelamo avatar Apr 19 '24 08:04 sdelamo

Our current workaround was rewriting all the involved HTTP client to reactive ones and then offload the fetching to custom schedulers with subscribeOn.

I've seen the solution with filter with authentication because I wondered how it is possible that it works.

There are workarounds but all of these makes using the framework very painful.

musketyr avatar Apr 19 '24 08:04 musketyr

There are workarounds but all of these makes using the framework very painful.

I understand but your application was blocking the Netty event loop and this exception pointed you to a necessary change. I don't think don't throwing an exception was a better alternative.

sdelamo avatar Apr 19 '24 09:04 sdelamo

but if I understand it properly, none of these would be an issue when the app will be fully using virtual threads, isn't it? so wouldn't be better to have some flag to switch to the virtual threads globally much better solution?

I would still prefer having a error logged for at least one minor release before throwing the error - something like Gradle is doing. throwing an error might be nice when developing new app but for large legacy system is a nightmare. what if there is a blocking call somewhere hidden and we don't have a valid test case for it yet. logging an error is something that can't be easily ignored if you use tools like Sentry but it still won't affect the user.

musketyr avatar Apr 19 '24 09:04 musketyr

Moving everything to virtual threads by default isn't happening anytime soon.

What you could instead try is add an empty request filter that has @ExecuteOn(BLOCKING). I think this should move the arg binders to the virtual thread too.

yawkat avatar Apr 19 '24 11:04 yawkat

thank you @yawkat for sharing the trick. indeed having a filter that executes on BLOCKING sends everything into the BLOCKING event loop if not declared otherwise

import io.micronaut.http.HttpRequest;
import io.micronaut.http.annotation.RequestFilter;
import io.micronaut.http.annotation.ServerFilter;
import io.micronaut.scheduling.TaskExecutors;
import io.micronaut.scheduling.annotation.ExecuteOn;

@ServerFilter("/**")
public class MoveToBlockingFilter {

    @RequestFilter
    @ExecuteOn(TaskExecutors.BLOCKING)
    public void filter(HttpRequest<?> request) {
        System.out.println("Filtering request " + request + " on blocking thread pool: " + Thread.currentThread().getName());
    }

}

musketyr avatar Apr 19 '24 12:04 musketyr

I think with the current design we can't support argument binders that block. The solution to this would be to move the blocking logic in the binder into a filter and in the filter set a request attribute then use this request attribute in the binder. That would provide the behaviour you are looking for.

graemerocher avatar May 27 '24 10:05 graemerocher

i think it makes sense in the long term though. we already have some arg binders that kind of block, such as @Body, which use weird PendingRequestBindingResult api. i think it may be worthwhile to move that to ExecutionFlow.

yawkat avatar May 27 '24 10:05 yawkat