From 73d30dd875affac7dc69232bf4c6b183e0801c26 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Wed, 3 May 2023 20:49:20 +0100 Subject: [PATCH] Polishing contribution Closes gh-30403 --- .../ROOT/pages/integration/rest-clients.adoc | 10 +++-- .../modules/ROOT/pages/rsocket.adoc | 10 +++-- .../rsocket/service/RSocketServiceMethod.java | 18 +++++---- .../service/RSocketServiceProxyFactory.java | 14 ++++--- .../service/invoker/HttpServiceMethod.java | 38 ++++++++----------- .../invoker/HttpServiceProxyFactory.java | 13 ++++--- 6 files changed, 53 insertions(+), 50 deletions(-) diff --git a/framework-docs/modules/ROOT/pages/integration/rest-clients.adoc b/framework-docs/modules/ROOT/pages/integration/rest-clients.adoc index b086fe5536..5860051578 100644 --- a/framework-docs/modules/ROOT/pages/integration/rest-clients.adoc +++ b/framework-docs/modules/ROOT/pages/integration/rest-clients.adoc @@ -497,10 +497,12 @@ Annotated, HTTP exchange methods support the following return values: TIP: You can also use any other async or reactive types registered in the `ReactiveAdapterRegistry`. -TIP: For non-reactive types, blocking from a reactive publisher is performed -under the hood by the framework. By default, it is done without a timeout. -You can set a timeout for blocking by calling `blockTimeout(Duration blockTimeout)` -on `HttpServiceProxyFactory.Builder`. +By default, the behavior of HTTP service methods with synchronous (blocking) method +signature depends on connection and timeout settings of the underlying HTTP client. +`HttpServiceProxyFactory.Builder` does expose a `blockTimeout` option that also lets you +configure the maximum time to block for a response, but we recommend configuring timeout +values directly on the underlying HTTP client, which likely provides more control over +such settings. [[rest-http-interface-exceptions]] diff --git a/framework-docs/modules/ROOT/pages/rsocket.adoc b/framework-docs/modules/ROOT/pages/rsocket.adoc index 6de50b2612..d30d9d34c3 100644 --- a/framework-docs/modules/ROOT/pages/rsocket.adoc +++ b/framework-docs/modules/ROOT/pages/rsocket.adoc @@ -1064,7 +1064,9 @@ Annotated, RSocket exchange methods support return values that are concrete valu any producer of value(s) that can be adapted to a Reactive Streams `Publisher` via `ReactiveAdapterRegistry`. -TIP: For non-reactive types, blocking from a reactive publisher is performed -under the hood by the framework. By default, it is done without a timeout. -You can set a timeout for blocking by calling `blockTimeout(Duration blockTimeout)` -on `RSocketServiceProxyFactory.Builder`. \ No newline at end of file +By default, the behavior of RSocket service methods with synchronous (blocking) method +signature depends on response timeout settings of the underlying RSocket `ClientTransport` +as well as RSocket keep-alive settings. `RSocketServiceProxyFactory.Builder` does expose a +`blockTimeout` option that also lets you configure the maximum time to block for a response, +but we recommend configuring timeout values at the RSocket level for more control. + diff --git a/spring-messaging/src/main/java/org/springframework/messaging/rsocket/service/RSocketServiceMethod.java b/spring-messaging/src/main/java/org/springframework/messaging/rsocket/service/RSocketServiceMethod.java index db26f744f8..4b1f3e8f8d 100644 --- a/spring-messaging/src/main/java/org/springframework/messaging/rsocket/service/RSocketServiceMethod.java +++ b/spring-messaging/src/main/java/org/springframework/messaging/rsocket/service/RSocketServiceMethod.java @@ -73,8 +73,7 @@ final class RSocketServiceMethod { this.parameters = initMethodParameters(method); this.argumentResolvers = argumentResolvers; this.route = initRoute(method, containingClass, rsocketRequester.strategies(), embeddedValueResolver); - this.responseFunction = initResponseFunction( - rsocketRequester, method, reactiveRegistry, blockTimeout); + this.responseFunction = initResponseFunction(rsocketRequester, method, reactiveRegistry, blockTimeout); } private static MethodParameter[] initMethodParameters(Method method) { @@ -163,11 +162,16 @@ final class RSocketServiceMethod { if (reactiveAdapter != null) { return reactiveAdapter.fromPublisher(responsePublisher); } - return (blockForOptional ? - (blockTimeout != null ? ((Mono) responsePublisher).blockOptional(blockTimeout) : - ((Mono) responsePublisher).blockOptional()) : - (blockTimeout != null ? ((Mono) responsePublisher).block(blockTimeout) : - ((Mono) responsePublisher).block())); + if (blockForOptional) { + return (blockTimeout != null ? + ((Mono) responsePublisher).blockOptional(blockTimeout) : + ((Mono) responsePublisher).blockOptional()); + } + else { + return (blockTimeout != null ? + ((Mono) responsePublisher).block(blockTimeout) : + ((Mono) responsePublisher).block()); + } }); } diff --git a/spring-messaging/src/main/java/org/springframework/messaging/rsocket/service/RSocketServiceProxyFactory.java b/spring-messaging/src/main/java/org/springframework/messaging/rsocket/service/RSocketServiceProxyFactory.java index 076cb862b2..f0e08fb515 100644 --- a/spring-messaging/src/main/java/org/springframework/messaging/rsocket/service/RSocketServiceProxyFactory.java +++ b/spring-messaging/src/main/java/org/springframework/messaging/rsocket/service/RSocketServiceProxyFactory.java @@ -188,14 +188,17 @@ public final class RSocketServiceProxyFactory { } /** - * Configure how long to wait for a response for an HTTP service method + * Configure how long to block for the response of an RSocket service method * with a synchronous (blocking) method signature. - *

By default this is {@code null}, - * in which case means blocking on publishers is done without a timeout. + *

By default this is not set, in which case the behavior depends on + * connection and response timeout settings of the underlying RSocket + * {@code ClientTransport} as well as RSocket keep-alive settings. + * We recommend configuring timeout values at the RSocket level which + * provides more control. * @param blockTimeout the timeout value * @return this same builder instance */ - public Builder blockTimeout(Duration blockTimeout) { + public Builder blockTimeout(@Nullable Duration blockTimeout) { this.blockTimeout = blockTimeout; return this; } @@ -208,8 +211,7 @@ public final class RSocketServiceProxyFactory { return new RSocketServiceProxyFactory( this.rsocketRequester, initArgumentResolvers(), - this.embeddedValueResolver, this.reactiveAdapterRegistry, - this.blockTimeout); + this.embeddedValueResolver, this.reactiveAdapterRegistry, this.blockTimeout); } private List initArgumentResolvers() { diff --git a/spring-web/src/main/java/org/springframework/web/service/invoker/HttpServiceMethod.java b/spring-web/src/main/java/org/springframework/web/service/invoker/HttpServiceMethod.java index 2a01b0baee..1f04c99333 100644 --- a/spring-web/src/main/java/org/springframework/web/service/invoker/HttpServiceMethod.java +++ b/spring-web/src/main/java/org/springframework/web/service/invoker/HttpServiceMethod.java @@ -123,15 +123,13 @@ final class HttpServiceMethod { } } int index = i; - Assert.state(resolved, () -> formatArgumentError(this.parameters[index], "No suitable resolver")); + Assert.state(resolved, () -> + "Could not resolve parameter [" + this.parameters[index].getParameterIndex() + "] in " + + this.parameters[index].getExecutable().toGenericString() + + (StringUtils.hasText("No suitable resolver") ? ": " + "No suitable resolver" : "")); } } - private static String formatArgumentError(MethodParameter param, String message) { - return "Could not resolve parameter [" + param.getParameterIndex() + "] in " + - param.getExecutable().toGenericString() + (StringUtils.hasText(message) ? ": " + message : ""); - } - /** * Factory for {@link HttpRequestValues} with values extracted from the type @@ -277,17 +275,6 @@ final class HttpServiceMethod { @Nullable ReactiveAdapter returnTypeAdapter, boolean blockForOptional, @Nullable Duration blockTimeout) { - private ResponseFunction( - Function> responseFunction, - @Nullable ReactiveAdapter returnTypeAdapter, - boolean blockForOptional, Duration blockTimeout) { - - this.responseFunction = responseFunction; - this.returnTypeAdapter = returnTypeAdapter; - this.blockForOptional = blockForOptional; - this.blockTimeout = blockTimeout; - } - @Nullable public Object execute(HttpRequestValues requestValues) { @@ -297,11 +284,16 @@ final class HttpServiceMethod { return this.returnTypeAdapter.fromPublisher(responsePublisher); } - return (this.blockForOptional ? - (this.blockTimeout != null ? ((Mono) responsePublisher).blockOptional(this.blockTimeout) : - ((Mono) responsePublisher).blockOptional()) : - (this.blockTimeout != null ? ((Mono) responsePublisher).block(this.blockTimeout) : - ((Mono) responsePublisher).block())); + if (this.blockForOptional) { + return (this.blockTimeout != null ? + ((Mono) responsePublisher).blockOptional(this.blockTimeout) : + ((Mono) responsePublisher).blockOptional()); + } + else { + return (this.blockTimeout != null ? + ((Mono) responsePublisher).block(this.blockTimeout) : + ((Mono) responsePublisher).block()); + } } @@ -310,7 +302,7 @@ final class HttpServiceMethod { */ public static ResponseFunction create( HttpClientAdapter client, Method method, ReactiveAdapterRegistry reactiveRegistry, - Duration blockTimeout) { + @Nullable Duration blockTimeout) { MethodParameter returnParam = new MethodParameter(method, -1); Class returnType = returnParam.getParameterType(); diff --git a/spring-web/src/main/java/org/springframework/web/service/invoker/HttpServiceProxyFactory.java b/spring-web/src/main/java/org/springframework/web/service/invoker/HttpServiceProxyFactory.java index 51bb1e285b..5d9a2ac81b 100644 --- a/spring-web/src/main/java/org/springframework/web/service/invoker/HttpServiceProxyFactory.java +++ b/spring-web/src/main/java/org/springframework/web/service/invoker/HttpServiceProxyFactory.java @@ -207,14 +207,16 @@ public final class HttpServiceProxyFactory { } /** - * Configure how long to wait for a response for an HTTP service method + * Configure how long to block for the response of an HTTP service method * with a synchronous (blocking) method signature. - *

By default this is {@code null}, - * in which case means blocking on publishers is done without a timeout. + *

By default this is not set, in which case the behavior depends on + * connection and request timeout settings of the underlying HTTP client. + * We recommend configuring timeout values directly on the underlying HTTP + * client, which provides more control over such settings. * @param blockTimeout the timeout value * @return this same builder instance */ - public Builder blockTimeout(Duration blockTimeout) { + public Builder blockTimeout(@Nullable Duration blockTimeout) { this.blockTimeout = blockTimeout; return this; } @@ -227,8 +229,7 @@ public final class HttpServiceProxyFactory { return new HttpServiceProxyFactory( this.clientAdapter, initArgumentResolvers(), - this.embeddedValueResolver, this.reactiveAdapterRegistry, - this.blockTimeout); + this.embeddedValueResolver, this.reactiveAdapterRegistry, this.blockTimeout); } private List initArgumentResolvers() {