Timeout added to WebFluxMessageHandlerSpec.java & WebFluxRequestExecutingMessageHandler.java #3188

sirimamilla · 2020-02-20T16:26:59Z

Timeout added to WebFluxMessageHandlerSpec.java & WebFluxRequestExecutingMessageHandler.java #3183

Repushed from GH-3183

…tingMessageHandler.java spring-projects#3183

…-3183 # Conflicts: # src/reference/asciidoc/whats-new.adoc

…tingMessageHandler.java spring-projects#3183

artembilan · 2020-02-20T16:29:43Z

Thanks, but see the last sentence in the CONTRIBUTION guideline:

Please, follow Chris Beams' recommendations in regards to the good commit message

Having so many contribution from you would benefit from a good commit message formatting.

😄

sirimamilla · 2020-02-20T16:37:33Z

Please, follow Chris Beams' recommendations in regards to the good commit message

Having so many contribution from you would benefit from a good commit message formatting.

I will have a look at this and follow this. I have little time to contribute for this, so created a PR. Will look at your review comments tomorrow. Good Night.

Hope we can agree on this Feature Enhancement. I am working on adding transform method as well as you suggested, I would prefer to raise a separate PR for transform.

Also, I am thinking of right usecase for adding it in the test cases for the transform message.

artembilan

Please, consider to expose this new request-timeout attribute in the XML configuration. See spring-integration-webflux.xsd and respective WebFluxOutboundChannelAdapterParser.
Then you can find a WebFluxOutboundChannelAdapterParserTests to verify that new option is applied properly.

You can also add your name to all the affected classes.

Thank you!

artembilan · 2020-02-21T14:57:08Z

...lux/src/main/java/org/springframework/integration/webflux/dsl/WebFluxMessageHandlerSpec.java

@@ -144,6 +145,41 @@ public WebFluxMessageHandlerSpec publisherElementTypeExpression(Expression publi
 		return this;
 	}

+	/**
+	 * Specify the timeout value for receiving the response form the server.


Typo: from ? And that has to be an "a timeout"

artembilan · 2020-02-21T14:57:24Z

...lux/src/main/java/org/springframework/integration/webflux/dsl/WebFluxMessageHandlerSpec.java

@@ -144,6 +145,41 @@ public WebFluxMessageHandlerSpec publisherElementTypeExpression(Expression publi
 		return this;
 	}

+	/**
+	 * Specify the timeout value for receiving the response form the server.
+	 * If the response is not received with in timeout value, will result in Timeout Exception


artembilan · 2020-02-21T15:00:06Z

...lux/src/main/java/org/springframework/integration/webflux/dsl/WebFluxMessageHandlerSpec.java

+	 * @return the spec
+	 * @since 5.3
+	 */
+	public WebFluxMessageHandlerSpec timeout(long timeoutInMillis) {


That's bad name. There are many aspects in Spring Integration components and a lot of different timeout options.
Would be great to be more specific here. How about a requestTimeout ?

Also: I wouldn't expose this long option. It is not too hard to have that Duration.ofMillis() in the target application.
Plus with Spring Boot configuration properties we can provide an option in the duration style and we'll get exactly Duration instance in the POJO

artembilan · 2020-02-21T15:01:38Z

.../org/springframework/integration/webflux/outbound/WebFluxRequestExecutingMessageHandler.java

+	 * @param timeout accepts {@link java.time.Duration}
+	 * @since 5.3
+	 */
+	public void setTimeout(Duration timeout) {


I understand that this is simple one, but would be better to not have it since a Function can simple cover the static value as well.

So, please, remove

artembilan · 2020-02-21T15:04:43Z

.../org/springframework/integration/webflux/outbound/WebFluxRequestExecutingMessageHandler.java

+	 * If the response is not received with in timeout value, will result in Timeout Exception
+	 * @since 5.3
+	 */
+	public void setTimeoutFunction(Function<Message<?>, Duration> timeoutFunction) {


Well, we prefer an Expression variant instead of function. This way a timeout could be configured in the XML an easy way.
There is a FunctionExpression to cover this case. And exactly Java DSL should expose a function-based option. Of course, alongside with an expression. See publisherElementTypeFunction in the modified by you WebFluxMessageHandlerSpec

artembilan · 2020-02-21T15:15:06Z

...ation-webflux/src/test/java/org/springframework/integration/webflux/dsl/WebFluxDslTests.java

+
+		Message<String> testMessage =
+				MessageBuilder.withPayload("test")
+						.setReplyChannel(replyChannel)


I think we shouldn't use a setReplyChannel for this kind of test. Since you use the same instance for both reply and error responses, I was confused first about exception assertion in the end. I was expected from "error channel", but I saw a replyChannel. So, your test-case mislead me to investigate how do we process exceptions like that timeout 😢

artembilan · 2020-02-21T15:19:46Z

src/reference/asciidoc/whats-new.adoc

+=== WebFlux Changes
+
+The `timeoutFunction` property is added to `WebFluxRequestExecutingMessageHandler`.
+Java DSL accessor which accepts long Timeout, Duration Timeout & Timeout Function is provided


The good sentence ends with period.
I don't think that word "timeout" deserves to be capitalized.
I would even say if your don't talk about code, then even function word doesn't deserve to be capitalized.

Would be great to have here a brief description for the reason of this new option. See something already existed in this doc.
Then we definitely need a link to the target docs.
And what is very important we need a description in the final docs - webflux.adoc.

artembilan · 2020-02-24T16:03:06Z

I don't see changes and your comment in the PR, but if you talk about request-timeout on the inbound-gateway, that's fully different story and should not confuse with alike name on the outbound-gateway.
So, I still insists to call it a requestTimeout everywhere what is based on the WebFluxRequestExecutingMessageHandler.

sirimamilla · 2020-02-24T23:08:16Z

Hi @artembilan ,

I was busy with office activities, I am almost done with my changes. I will push changes today

artembilan · 2020-02-26T15:21:04Z

@sirimamilla ,

you know after watching yesterday Josh Long's Reactive Revolution presentation I have to take my decision back. There are indeed much more control and aspect operators in the Reactor Flux & Mono.
That is your use-case to go with a simple timeout. Other people would like to do a retry(), onErrorResume() or even a Circuit Breaker: https://spring.io/projects/spring-cloud-circuitbreaker

So, for me it sounds more like a general solution for any MessageHandler which produces a reactive type as a result. Therefore I think about something like ReactiveRequestHandlerAdvice to apply for any endpoint with reactive capabilities.
That's definitely not only a WebFlux and not only a timeout concern to not have some general solution for anything reactive-based what can have problems connecting to remote part.

So, starting from here it is 👎 from me for this PR: a general solution with a ReactiveRequestHandlerAdvice and injected BiFunction into it for any possible returned reactive type customization will cover this timeout concern as well.

sirimamilla · 2020-02-26T23:28:36Z

Hi @artembilan,
Your solution of providing an advice seems more generic. I am ok with that solution as it can solve more problems

artembilan · 2020-02-27T21:42:19Z

Closed in favor of #3197

@sirimamilla , you are feel free to provide any feedback over there.

Thank you for your contribution!

sirimamilla added 3 commits February 20, 2020 23:51

Timeout added to WebFluxMessageHandlerSpec.java & WebFluxRequestExecu…

a32e1df

…tingMessageHandler.java spring-projects#3183

Merge remote-tracking branch 'upstream/master' into spring-projectsGH…

49e0d9e

…-3183 # Conflicts: # src/reference/asciidoc/whats-new.adoc

Timeout added to WebFluxMessageHandlerSpec.java & WebFluxRequestExecu…

6dfdf0a

…tingMessageHandler.java spring-projects#3183

sirimamilla requested a review from artembilan February 20, 2020 16:27

sirimamilla mentioned this pull request Feb 20, 2020

Timeout added to WebFluxMessageHandlerSpec.java & WebFluxRequestExecutingMessageHandler.java #3187

Closed

artembilan linked an issue Feb 21, 2020 that may be closed by this pull request

Add hooks for reactive reply customizations: timeout(), retry(), tag() etc. #3183

Closed

artembilan requested changes Feb 21, 2020

View reviewed changes

artembilan mentioned this pull request Feb 26, 2020

Add hooks for reactive reply customizations: timeout(), retry(), tag() etc. #3183

Closed

artembilan closed this Feb 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timeout added to WebFluxMessageHandlerSpec.java & WebFluxRequestExecutingMessageHandler.java #3188

Timeout added to WebFluxMessageHandlerSpec.java & WebFluxRequestExecutingMessageHandler.java #3188

sirimamilla commented Feb 20, 2020

artembilan commented Feb 20, 2020

sirimamilla commented Feb 20, 2020

artembilan left a comment

artembilan Feb 21, 2020

artembilan Feb 21, 2020

artembilan Feb 21, 2020

artembilan Feb 21, 2020

artembilan Feb 21, 2020

artembilan Feb 21, 2020

artembilan Feb 21, 2020

artembilan commented Feb 24, 2020

sirimamilla commented Feb 24, 2020

artembilan commented Feb 26, 2020

sirimamilla commented Feb 26, 2020

artembilan commented Feb 27, 2020

Timeout added to WebFluxMessageHandlerSpec.java & WebFluxRequestExecutingMessageHandler.java #3188

Timeout added to WebFluxMessageHandlerSpec.java & WebFluxRequestExecutingMessageHandler.java #3188

Conversation

sirimamilla commented Feb 20, 2020

artembilan commented Feb 20, 2020

sirimamilla commented Feb 20, 2020

artembilan left a comment

Choose a reason for hiding this comment

artembilan Feb 21, 2020

Choose a reason for hiding this comment

artembilan Feb 21, 2020

Choose a reason for hiding this comment

artembilan Feb 21, 2020

Choose a reason for hiding this comment

artembilan Feb 21, 2020

Choose a reason for hiding this comment

artembilan Feb 21, 2020

Choose a reason for hiding this comment

artembilan Feb 21, 2020

Choose a reason for hiding this comment

artembilan Feb 21, 2020

Choose a reason for hiding this comment

artembilan commented Feb 24, 2020

sirimamilla commented Feb 24, 2020

artembilan commented Feb 26, 2020

sirimamilla commented Feb 26, 2020

artembilan commented Feb 27, 2020