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

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

Open
musketyr opened this issue Apr 19, 2024 · 11 comments
Assignees
Labels
status: next major version The issue will be considered for the next major version

Comments

@musketyr
Copy link
Contributor

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

@sdelamo
Copy link
Collaborator

sdelamo commented Apr 19, 2024

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.

@musketyr
Copy link
Contributor Author

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]

@sdelamo
Copy link
Collaborator

sdelamo commented Apr 19, 2024

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

@sdelamo
Copy link
Collaborator

sdelamo commented Apr 19, 2024

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.

@musketyr
Copy link
Contributor Author

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.

@sdelamo
Copy link
Collaborator

sdelamo commented Apr 19, 2024

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.

@musketyr
Copy link
Contributor Author

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.

@yawkat
Copy link
Member

yawkat commented Apr 19, 2024

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.

@musketyr
Copy link
Contributor Author

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());
    }

}

@graemerocher graemerocher added the closed: won't fix The issue will not be worked on label May 27, 2024
@graemerocher
Copy link
Contributor

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.

@yawkat yawkat added status: next major version The issue will be considered for the next major version and removed closed: won't fix The issue will not be worked on labels May 27, 2024
@yawkat
Copy link
Member

yawkat commented May 27, 2024

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 yawkat reopened this May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: next major version The issue will be considered for the next major version
Projects
None yet
Development

No branches or pull requests

4 participants