From 521cda009b851f0f46985953f6f9e98599db776d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Wed, 3 Apr 2024 19:22:02 +0200 Subject: [PATCH 1/3] Use non-blocking thread in WebFlux controller with RequestBody parameter This commit ensures that `InvocableHandlerMethod` executes the method on the desired thread if a non-blocking thread is specified, even in the case where arguments resolution happens on a different thread. This is notably the case if the method body is resolved as an input argument to the controller method (`@RequestBody`). Closes gh-32502 --- .../result/method/InvocableHandlerMethod.java | 26 +++- .../annotation/ControllerMethodResolver.java | 31 ++++- .../RequestMappingHandlerAdapter.java | 11 +- .../method/InvocableHandlerMethodTests.java | 47 +++++++- .../annotation/ControllerAdviceTests.java | 113 ++++++++++++++++-- .../ControllerMethodResolverTests.java | 2 +- .../annotation/ModelInitializerTests.java | 3 +- .../RequestMappingIntegrationTests.java | 6 +- .../annotation/ModelInitializerKotlinTests.kt | 2 +- 9 files changed, 217 insertions(+), 24 deletions(-) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/InvocableHandlerMethod.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/InvocableHandlerMethod.java index 8bc8dd8dfa..e9d7a33b01 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/InvocableHandlerMethod.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/InvocableHandlerMethod.java @@ -36,6 +36,7 @@ import kotlin.reflect.full.KClasses; import kotlin.reflect.jvm.KCallablesJvm; import kotlin.reflect.jvm.ReflectJvmMapping; import reactor.core.publisher.Mono; +import reactor.core.scheduler.Scheduler; import org.springframework.core.CoroutinesUtils; import org.springframework.core.DefaultParameterNameDiscoverer; @@ -59,6 +60,12 @@ import org.springframework.web.server.ServerWebExchange; * Extension of {@link HandlerMethod} that invokes the underlying method with * argument values resolved from the current HTTP request through a list of * {@link HandlerMethodArgumentResolver}. + *

By default, the method invocation happens on the thread from which the + * {@code Mono} was subscribed to, or in some cases the thread that emitted one + * of the resolved arguments (e.g. when the request body needs to be decoded). + * To ensure a predictable thread for the underlying method's invocation, + * a {@link Scheduler} can optionally be provided via + * {@link #setInvocationScheduler(Scheduler)}. * * @author Rossen Stoyanchev * @author Juergen Hoeller @@ -85,6 +92,9 @@ public class InvocableHandlerMethod extends HandlerMethod { private Class[] validationGroups = EMPTY_GROUPS; + @Nullable + private Scheduler invocationScheduler; + /** * Create an instance from a {@code HandlerMethod}. @@ -153,6 +163,13 @@ public class InvocableHandlerMethod extends HandlerMethod { methodValidator.determineValidationGroups(getBean(), getBridgedMethod()) : EMPTY_GROUPS); } + /** + * Set the {@link Scheduler} on which to perform the method invocation. + * @since 6.1.6 + */ + public void setInvocationScheduler(@Nullable Scheduler invocationScheduler) { + this.invocationScheduler = invocationScheduler; + } /** * Invoke the method for the given exchange. @@ -165,7 +182,7 @@ public class InvocableHandlerMethod extends HandlerMethod { public Mono invoke( ServerWebExchange exchange, BindingContext bindingContext, Object... providedArgs) { - return getMethodArgumentValues(exchange, bindingContext, providedArgs).flatMap(args -> { + return getMethodArgumentValuesOnScheduler(exchange, bindingContext, providedArgs).flatMap(args -> { if (shouldValidateArguments() && this.methodValidator != null) { this.methodValidator.applyArgumentValidation( getBean(), getBridgedMethod(), getMethodParameters(), args, this.validationGroups); @@ -217,6 +234,12 @@ public class InvocableHandlerMethod extends HandlerMethod { }); } + private Mono getMethodArgumentValuesOnScheduler( + ServerWebExchange exchange, BindingContext bindingContext, Object... providedArgs) { + Mono argumentValuesMono = getMethodArgumentValues(exchange, bindingContext, providedArgs); + return this.invocationScheduler != null ? argumentValuesMono.publishOn(this.invocationScheduler) : argumentValuesMono; + } + private Mono getMethodArgumentValues( ServerWebExchange exchange, BindingContext bindingContext, Object... providedArgs) { @@ -224,7 +247,6 @@ public class InvocableHandlerMethod extends HandlerMethod { if (ObjectUtils.isEmpty(parameters)) { return EMPTY_ARGS; } - List> argMonos = new ArrayList<>(parameters.length); for (MethodParameter parameter : parameters) { parameter.initParameterNameDiscovery(this.parameterNameDiscoverer); diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ControllerMethodResolver.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ControllerMethodResolver.java index 18e380178d..04158378f8 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ControllerMethodResolver.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ControllerMethodResolver.java @@ -28,6 +28,7 @@ import java.util.function.Predicate; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import reactor.core.scheduler.Scheduler; import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; import org.springframework.context.ApplicationContext; @@ -121,11 +122,19 @@ class ControllerMethodResolver { private final Map, SessionAttributesHandler> sessionAttributesHandlerCache = new ConcurrentHashMap<>(64); + @Nullable + private final Scheduler invocationScheduler; + + @Nullable + private final Predicate blockingMethodPredicate; + ControllerMethodResolver( ArgumentResolverConfigurer customResolvers, ReactiveAdapterRegistry adapterRegistry, ConfigurableApplicationContext context, List> readers, - @Nullable WebBindingInitializer webBindingInitializer) { + @Nullable WebBindingInitializer webBindingInitializer, + @Nullable Scheduler invocationScheduler, + @Nullable Predicate blockingMethodPredicate) { Assert.notNull(customResolvers, "ArgumentResolverConfigurer is required"); Assert.notNull(adapterRegistry, "ReactiveAdapterRegistry is required"); @@ -137,6 +146,8 @@ class ControllerMethodResolver { this.requestMappingResolvers = requestMappingResolvers(customResolvers, adapterRegistry, context, readers); this.exceptionHandlerResolvers = exceptionHandlerResolvers(customResolvers, adapterRegistry, context); this.reactiveAdapterRegistry = adapterRegistry; + this.invocationScheduler = invocationScheduler; + this.blockingMethodPredicate = blockingMethodPredicate; if (BEAN_VALIDATION_PRESENT) { this.methodValidator = HandlerMethodValidator.from(webBindingInitializer, null, @@ -287,6 +298,21 @@ class ControllerMethodResolver { }; } + /** + * Return a {@link Scheduler} for the given method if it is considered + * blocking by the underlying blocking method predicate, or null if no + * particular scheduler should be used for this method invocation. + */ + @Nullable + public Scheduler getSchedulerFor(HandlerMethod handlerMethod) { + if (this.invocationScheduler != null) { + Assert.state(this.blockingMethodPredicate != null, "Expected HandlerMethod Predicate"); + if (this.blockingMethodPredicate.test(handlerMethod)) { + return this.invocationScheduler; + } + } + return null; + } /** * Return an {@link InvocableHandlerMethod} for the given @@ -297,6 +323,9 @@ class ControllerMethodResolver { invocable.setArgumentResolvers(this.requestMappingResolvers); invocable.setReactiveAdapterRegistry(this.reactiveAdapterRegistry); invocable.setMethodValidator(this.methodValidator); + //getSchedulerFor returns null if not applicable, which is ok here + invocable.setInvocationScheduler(getSchedulerFor(handlerMethod)); + return invocable; } diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerAdapter.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerAdapter.java index 5e8167d8b1..7098dcd96e 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerAdapter.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerAdapter.java @@ -225,7 +225,8 @@ public class RequestMappingHandlerAdapter this.methodResolver = new ControllerMethodResolver( this.argumentResolverConfigurer, this.reactiveAdapterRegistry, this.applicationContext, - this.messageReaders, this.webBindingInitializer); + this.messageReaders, this.webBindingInitializer, + this.scheduler, this.blockingMethodPredicate); this.modelInitializer = new ModelInitializer(this.methodResolver, this.reactiveAdapterRegistry); } @@ -260,11 +261,9 @@ public class RequestMappingHandlerAdapter .doOnNext(result -> result.setExceptionHandler(exceptionHandler)) .onErrorResume(ex -> exceptionHandler.handleError(exchange, ex)); - if (this.scheduler != null) { - Assert.state(this.blockingMethodPredicate != null, "Expected HandlerMethod Predicate"); - if (this.blockingMethodPredicate.test(handlerMethod)) { - resultMono = resultMono.subscribeOn(this.scheduler); - } + Scheduler optionalScheduler = this.methodResolver.getSchedulerFor(handlerMethod); + if (optionalScheduler != null) { + return resultMono.subscribeOn(optionalScheduler); } return resultMono; diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/InvocableHandlerMethodTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/InvocableHandlerMethodTests.java index 1328266090..cf51816dd2 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/InvocableHandlerMethodTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/InvocableHandlerMethodTests.java @@ -26,6 +26,8 @@ import java.util.List; import org.junit.jupiter.api.Test; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; +import reactor.core.scheduler.Scheduler; +import reactor.core.scheduler.Schedulers; import reactor.test.StepVerifier; import org.springframework.core.io.buffer.DataBuffer; @@ -75,6 +77,15 @@ class InvocableHandlerMethodTests { assertHandlerResultValue(mono, "success:value1"); } + @Test + void resolveArgOnSchedulerThread() { + this.resolvers.add(stubResolver(Mono.just("success").publishOn(Schedulers.newSingle("wrong")))); + Method method = ResolvableMethod.on(TestController.class).mockCall(o -> o.singleArgThread(null)).method(); + Mono mono = invokeOnScheduler(Schedulers.newSingle("good"), new TestController(), method); + + assertHandlerResultValue(mono, "success on thread: good-", false); + } + @Test void resolveNoArgValue() { this.resolvers.add(stubResolver(Mono.empty())); @@ -92,6 +103,14 @@ class InvocableHandlerMethodTests { assertHandlerResultValue(mono, "success"); } + @Test + void resolveNoArgsOnSchedulerThread() { + Method method = ResolvableMethod.on(TestController.class).mockCall(o -> o.noArgsThread()).method(); + Mono mono = invokeOnScheduler(Schedulers.newSingle("good"), new TestController(), method); + + assertHandlerResultValue(mono, "on thread: good-", false); + } + @Test void cannotResolveArg() { Method method = ResolvableMethod.on(TestController.class).mockCall(o -> o.singleArg(null)).method(); @@ -229,6 +248,13 @@ class InvocableHandlerMethodTests { return invocable.invoke(this.exchange, new BindingContext(), providedArgs); } + private Mono invokeOnScheduler(Scheduler scheduler, Object handler, Method method, Object... providedArgs) { + InvocableHandlerMethod invocable = new InvocableHandlerMethod(handler, method); + invocable.setArgumentResolvers(this.resolvers); + invocable.setInvocationScheduler(scheduler); + return invocable.invoke(this.exchange, new BindingContext(), providedArgs); + } + private HandlerMethodArgumentResolver stubResolver(Object stubValue) { return stubResolver(Mono.just(stubValue)); } @@ -241,8 +267,19 @@ class InvocableHandlerMethodTests { } private void assertHandlerResultValue(Mono mono, String expected) { + this.assertHandlerResultValue(mono, expected, true); + } + + private void assertHandlerResultValue(Mono mono, String expected, boolean strict) { StepVerifier.create(mono) - .consumeNextWith(result -> assertThat(result.getReturnValue()).isEqualTo(expected)) + .assertNext(result -> { + if (strict) { + assertThat(result.getReturnValue()).isEqualTo(expected); + } + else { + assertThat(String.valueOf(result.getReturnValue())).startsWith(expected); + } + }) .expectComplete() .verify(); } @@ -259,6 +296,14 @@ class InvocableHandlerMethodTests { return "success"; } + String singleArgThread(String q) { + return q + " on thread: " + Thread.currentThread().getName(); + } + + String noArgsThread() { + return "on thread: " + Thread.currentThread().getName(); + } + void exceptionMethod() { throw new IllegalStateException("boo"); } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ControllerAdviceTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ControllerAdviceTests.java index fc95d3ebe3..1163a020b8 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ControllerAdviceTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ControllerAdviceTests.java @@ -19,8 +19,11 @@ package org.springframework.web.reactive.result.method.annotation; import java.lang.reflect.Method; import java.time.Duration; import java.util.Collections; +import java.util.concurrent.Executors; import org.junit.jupiter.api.Test; +import reactor.core.publisher.Flux; +import reactor.core.scheduler.Schedulers; import org.springframework.beans.FatalBeanException; import org.springframework.context.ApplicationContext; @@ -28,6 +31,8 @@ import org.springframework.context.annotation.AnnotationConfigApplicationContext import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.core.annotation.Order; +import org.springframework.core.io.buffer.DataBuffer; +import org.springframework.core.io.buffer.DefaultDataBufferFactory; import org.springframework.stereotype.Controller; import org.springframework.ui.Model; import org.springframework.util.ClassUtils; @@ -38,10 +43,13 @@ import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.InitBinder; import org.springframework.web.bind.annotation.ModelAttribute; +import org.springframework.web.bind.annotation.PostMapping; +import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.support.WebExchangeDataBinder; import org.springframework.web.method.HandlerMethod; import org.springframework.web.reactive.BindingContext; import org.springframework.web.reactive.HandlerResult; +import org.springframework.web.server.ServerWebExchange; import org.springframework.web.testfixture.http.server.reactive.MockServerHttpRequest; import org.springframework.web.testfixture.server.MockServerWebExchange; @@ -58,6 +66,14 @@ class ControllerAdviceTests { private final MockServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get("/")); + private final MockServerWebExchange postExchange = + MockServerWebExchange.from(MockServerHttpRequest.post("/") + .body(Flux.defer(() -> { + byte[] bytes = "request body".getBytes(); + DataBuffer buffer = DefaultDataBufferFactory.sharedInstance.wrap(bytes); + return Flux.just(buffer).subscribeOn(Schedulers.newSingle("bad thread")); + }))); + @Test void resolveExceptionGlobalHandler() throws Exception { @@ -88,15 +104,34 @@ class ControllerAdviceTests { testException(exception, rootCause.toString()); } - private void testException(Throwable exception, String expected) throws Exception { + @Test + void resolveOnAnotherThread() throws Exception { ApplicationContext context = new AnnotationConfigApplicationContext(TestConfig.class); - RequestMappingHandlerAdapter adapter = createAdapter(context); + final RequestMappingHandlerAdapter adapter = createAdapterWithExecutor(context, "good thread"); TestController controller = context.getBean(TestController.class); - controller.setException(exception); - Object actual = handle(adapter, controller, "handle").getReturnValue(); - assertThat(actual).isEqualTo(expected); + Object actual = handle(adapter, controller, this.postExchange, Duration.ofMillis(100), + "threadWithArg", String.class).getReturnValue(); + assertThat(actual).isEqualTo("request body from good thread"); + } + + @Test + void resolveEmptyOnAnotherThread() throws Exception { + ApplicationContext context = new AnnotationConfigApplicationContext(TestConfig.class); + final RequestMappingHandlerAdapter adapter = createAdapterWithExecutor(context, "good thread"); + + TestController controller = context.getBean(TestController.class); + + Object actual = handle(adapter, controller, this.postExchange, Duration.ofMillis(100), "thread") + .getReturnValue(); + assertThat(actual).isEqualTo("hello from good thread"); + } + + @Test + void resolveExceptionOnAnotherThread() throws Exception { + testException(new IllegalArgumentException(), "good thread", + "OneControllerAdvice: IllegalArgumentException on thread good thread"); } @Test @@ -128,6 +163,17 @@ class ControllerAdviceTests { } + private void testException(Throwable exception, String expected) throws Exception { + ApplicationContext context = new AnnotationConfigApplicationContext(TestConfig.class); + RequestMappingHandlerAdapter adapter = createAdapter(context); + + TestController controller = context.getBean(TestController.class); + controller.setException(exception); + + Object actual = handle(adapter, controller, "handle").getReturnValue(); + assertThat(actual).isEqualTo(expected); + } + private RequestMappingHandlerAdapter createAdapter(ApplicationContext context) throws Exception { RequestMappingHandlerAdapter adapter = new RequestMappingHandlerAdapter(); adapter.setApplicationContext(context); @@ -135,12 +181,43 @@ class ControllerAdviceTests { return adapter; } - private HandlerResult handle(RequestMappingHandlerAdapter adapter, - Object controller, String methodName) throws Exception { + private void testException(Throwable exception, String threadName, String expected) throws Exception { + ApplicationContext context = new AnnotationConfigApplicationContext(TestConfig.class); + RequestMappingHandlerAdapter adapter = createAdapterWithExecutor(context, threadName); - Method method = controller.getClass().getMethod(methodName); + TestController controller = context.getBean(TestController.class); + controller.setException(exception); + + Object actual = handle(adapter, controller, this.postExchange, Duration.ofMillis(100) + , "threadWithArg", String.class).getReturnValue(); + assertThat(actual).isEqualTo(expected); + } + + private RequestMappingHandlerAdapter createAdapterWithExecutor(ApplicationContext context, String threadName) throws Exception { + RequestMappingHandlerAdapter adapter = new RequestMappingHandlerAdapter(); + adapter.setApplicationContext(context); + adapter.setBlockingExecutor(Executors.newSingleThreadExecutor(r -> { + Thread t = new Thread(r); + t.setName(threadName); + return t; + })); + adapter.setBlockingMethodPredicate(m -> true); + adapter.afterPropertiesSet(); + return adapter; + } + + private HandlerResult handle(RequestMappingHandlerAdapter adapter, + Object controller, String methodName, Class... parameterTypes) throws Exception { + return handle(adapter, controller, this.exchange, Duration.ZERO, methodName, parameterTypes); + } + + private HandlerResult handle(RequestMappingHandlerAdapter adapter, + Object controller, ServerWebExchange exchange, Duration timeout, + String methodName, Class... parameterTypes) throws Exception { + + Method method = controller.getClass().getMethod(methodName, parameterTypes); HandlerMethod handlerMethod = new HandlerMethod(controller, method); - return adapter.handle(this.exchange, handlerMethod).block(Duration.ZERO); + return adapter.handle(exchange, handlerMethod).block(timeout); } @@ -198,6 +275,18 @@ class ControllerAdviceTests { throw this.exception; } } + + @PostMapping + public String threadWithArg(@RequestBody String body) throws Throwable { + handle(); + return body + " from " + Thread.currentThread().getName(); + } + + @GetMapping + public String thread() throws Throwable { + handle(); + return "hello from " + Thread.currentThread().getName(); + } } @ControllerAdvice @@ -220,6 +309,12 @@ class ControllerAdviceTests { return "HandlerMethod: " + handlerMethod.getMethod().getName(); } + @ExceptionHandler(IllegalArgumentException.class) + public String handleOnThread(IllegalArgumentException ex) { + return "OneControllerAdvice: " + ClassUtils.getShortName(ex.getClass()) + + " on thread " + Thread.currentThread().getName(); + } + @ExceptionHandler(AssertionError.class) public String handleAssertionError(Error err) { return err.toString(); diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ControllerMethodResolverTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ControllerMethodResolverTests.java index f76d0a09b9..1df47ab627 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ControllerMethodResolverTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ControllerMethodResolverTests.java @@ -77,7 +77,7 @@ class ControllerMethodResolverTests { this.methodResolver = new ControllerMethodResolver( resolvers, ReactiveAdapterRegistry.getSharedInstance(), applicationContext, - codecs.getReaders(), null); + codecs.getReaders(), null, null, null); Method method = ResolvableMethod.on(TestController.class).mockCall(TestController::handle).method(); this.handlerMethod = new HandlerMethod(new TestController(), method); diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ModelInitializerTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ModelInitializerTests.java index 3b05422072..675079f9cf 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ModelInitializerTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ModelInitializerTests.java @@ -80,7 +80,8 @@ class ModelInitializerTests { ControllerMethodResolver methodResolver = new ControllerMethodResolver( resolverConfigurer, adapterRegistry, new StaticApplicationContext(), - Collections.emptyList(), null); + Collections.emptyList(), null, + null, null); this.modelInitializer = new ModelInitializer(methodResolver, adapterRegistry); } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingIntegrationTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingIntegrationTests.java index b650ebe885..b1a9465676 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingIntegrationTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingIntegrationTests.java @@ -78,8 +78,10 @@ class RequestMappingIntegrationTests extends AbstractRequestMappingIntegrationTe url += "/"; assertThat(getRestTemplate().getForObject(url, String.class)).isEqualTo("root"); - assertThat(getApplicationContext().getBean(TestExecutor.class).invocationCount.get()).isEqualTo(2); - assertThat(getApplicationContext().getBean(TestPredicate.class).invocationCount.get()).isEqualTo(2); + assertThat(getApplicationContext().getBean(TestExecutor.class).invocationCount.get()) + .as("executor").isEqualTo(4); + assertThat(getApplicationContext().getBean(TestPredicate.class).invocationCount.get()) + .as("predicate").isEqualTo(4); } @ParameterizedHttpServerTest diff --git a/spring-webflux/src/test/kotlin/org/springframework/web/reactive/result/method/annotation/ModelInitializerKotlinTests.kt b/spring-webflux/src/test/kotlin/org/springframework/web/reactive/result/method/annotation/ModelInitializerKotlinTests.kt index 88223c7641..5de51c2e1d 100644 --- a/spring-webflux/src/test/kotlin/org/springframework/web/reactive/result/method/annotation/ModelInitializerKotlinTests.kt +++ b/spring-webflux/src/test/kotlin/org/springframework/web/reactive/result/method/annotation/ModelInitializerKotlinTests.kt @@ -53,7 +53,7 @@ class ModelInitializerKotlinTests { val resolverConfigurer = ArgumentResolverConfigurer() resolverConfigurer.addCustomResolver(ModelMethodArgumentResolver(adapterRegistry)) val methodResolver = ControllerMethodResolver(resolverConfigurer, adapterRegistry, StaticApplicationContext(), - emptyList(), null) + emptyList(), null, null, null) modelInitializer = ModelInitializer(methodResolver, adapterRegistry) } From b68f76c86e356044b6f88884172d7f025061c3a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Nicoll?= Date: Thu, 4 Apr 2024 14:40:31 +0200 Subject: [PATCH 2/3] Polish See gh-32502 --- .../result/method/InvocableHandlerMethod.java | 1 + .../annotation/ControllerMethodResolver.java | 16 +++++++--------- .../method/InvocableHandlerMethodTests.java | 2 +- .../method/annotation/ControllerAdviceTests.java | 4 +++- .../method/annotation/ModelInitializerTests.java | 3 +-- .../RequestMappingIntegrationTests.java | 8 +++----- .../annotation/ModelInitializerKotlinTests.kt | 2 +- 7 files changed, 17 insertions(+), 19 deletions(-) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/InvocableHandlerMethod.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/InvocableHandlerMethod.java index e9d7a33b01..d863e74ecf 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/InvocableHandlerMethod.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/InvocableHandlerMethod.java @@ -247,6 +247,7 @@ public class InvocableHandlerMethod extends HandlerMethod { if (ObjectUtils.isEmpty(parameters)) { return EMPTY_ARGS; } + List> argMonos = new ArrayList<>(parameters.length); for (MethodParameter parameter : parameters) { parameter.initParameterNameDiscovery(this.parameterNameDiscoverer); diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ControllerMethodResolver.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ControllerMethodResolver.java index 04158378f8..7fda733d8e 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ControllerMethodResolver.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ControllerMethodResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -104,6 +104,12 @@ class ControllerMethodResolver { private final ReactiveAdapterRegistry reactiveAdapterRegistry; + @Nullable + private final Scheduler invocationScheduler; + + @Nullable + private final Predicate blockingMethodPredicate; + @Nullable private final MethodValidator methodValidator; @@ -122,12 +128,6 @@ class ControllerMethodResolver { private final Map, SessionAttributesHandler> sessionAttributesHandlerCache = new ConcurrentHashMap<>(64); - @Nullable - private final Scheduler invocationScheduler; - - @Nullable - private final Predicate blockingMethodPredicate; - ControllerMethodResolver( ArgumentResolverConfigurer customResolvers, ReactiveAdapterRegistry adapterRegistry, @@ -323,9 +323,7 @@ class ControllerMethodResolver { invocable.setArgumentResolvers(this.requestMappingResolvers); invocable.setReactiveAdapterRegistry(this.reactiveAdapterRegistry); invocable.setMethodValidator(this.methodValidator); - //getSchedulerFor returns null if not applicable, which is ok here invocable.setInvocationScheduler(getSchedulerFor(handlerMethod)); - return invocable; } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/InvocableHandlerMethodTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/InvocableHandlerMethodTests.java index cf51816dd2..c906010880 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/InvocableHandlerMethodTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/InvocableHandlerMethodTests.java @@ -105,7 +105,7 @@ class InvocableHandlerMethodTests { @Test void resolveNoArgsOnSchedulerThread() { - Method method = ResolvableMethod.on(TestController.class).mockCall(o -> o.noArgsThread()).method(); + Method method = ResolvableMethod.on(TestController.class).mockCall(TestController::noArgsThread).method(); Mono mono = invokeOnScheduler(Schedulers.newSingle("good"), new TestController(), method); assertHandlerResultValue(mono, "on thread: good-", false); diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ControllerAdviceTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ControllerAdviceTests.java index 1163a020b8..0cbd20e909 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ControllerAdviceTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ControllerAdviceTests.java @@ -217,7 +217,9 @@ class ControllerAdviceTests { Method method = controller.getClass().getMethod(methodName, parameterTypes); HandlerMethod handlerMethod = new HandlerMethod(controller, method); - return adapter.handle(exchange, handlerMethod).block(timeout); + HandlerResult handlerResult = adapter.handle(exchange, handlerMethod).block(timeout); + assertThat(handlerResult).isNotNull(); + return handlerResult; } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ModelInitializerTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ModelInitializerTests.java index 675079f9cf..6f55870375 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ModelInitializerTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ModelInitializerTests.java @@ -80,8 +80,7 @@ class ModelInitializerTests { ControllerMethodResolver methodResolver = new ControllerMethodResolver( resolverConfigurer, adapterRegistry, new StaticApplicationContext(), - Collections.emptyList(), null, - null, null); + Collections.emptyList(), null, null, null); this.modelInitializer = new ModelInitializer(methodResolver, adapterRegistry); } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingIntegrationTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingIntegrationTests.java index b1a9465676..34d8a2cd6d 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingIntegrationTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingIntegrationTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -78,10 +78,8 @@ class RequestMappingIntegrationTests extends AbstractRequestMappingIntegrationTe url += "/"; assertThat(getRestTemplate().getForObject(url, String.class)).isEqualTo("root"); - assertThat(getApplicationContext().getBean(TestExecutor.class).invocationCount.get()) - .as("executor").isEqualTo(4); - assertThat(getApplicationContext().getBean(TestPredicate.class).invocationCount.get()) - .as("predicate").isEqualTo(4); + assertThat(getApplicationContext().getBean(TestExecutor.class).invocationCount.get()).isEqualTo(4); + assertThat(getApplicationContext().getBean(TestPredicate.class).invocationCount.get()).isEqualTo(4); } @ParameterizedHttpServerTest diff --git a/spring-webflux/src/test/kotlin/org/springframework/web/reactive/result/method/annotation/ModelInitializerKotlinTests.kt b/spring-webflux/src/test/kotlin/org/springframework/web/reactive/result/method/annotation/ModelInitializerKotlinTests.kt index 5de51c2e1d..9ece488448 100644 --- a/spring-webflux/src/test/kotlin/org/springframework/web/reactive/result/method/annotation/ModelInitializerKotlinTests.kt +++ b/spring-webflux/src/test/kotlin/org/springframework/web/reactive/result/method/annotation/ModelInitializerKotlinTests.kt @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. From d955549037e632b76c7b716855bffcd673ffda04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Nicoll?= Date: Thu, 4 Apr 2024 14:30:39 +0200 Subject: [PATCH 3/3] Clarify scope of exception See gh-29491 --- .../web/servlet/NoHandlerFoundException.java | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/NoHandlerFoundException.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/NoHandlerFoundException.java index 99362952fe..22ba7647b1 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/NoHandlerFoundException.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/NoHandlerFoundException.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -27,14 +27,11 @@ import org.springframework.http.ProblemDetail; import org.springframework.web.ErrorResponse; /** - * By default, when the DispatcherServlet can't find a handler for a request it - * sends a 404 response. However, if its property "throwExceptionIfNoHandlerFound" - * is set to {@code true} this exception is raised and may be handled with - * a configured HandlerExceptionResolver. + * Thrown when the {@link DispatcherServlet} can't find a handler for a request, + * which may be handled with a configured {@link HandlerExceptionResolver}. * * @author Brian Clozel * @since 4.0 - * @see DispatcherServlet#setThrowExceptionIfNoHandlerFound(boolean) * @see DispatcherServlet#noHandlerFound(HttpServletRequest, HttpServletResponse) */ @SuppressWarnings("serial")