Treat HttpClient timeouts the same as Hystrix timeouts #555

mmanciop · 2018-09-19T10:20:15Z

codecov-io · 2018-09-19T10:48:51Z

Codecov Report

Merging #555 into 2.0.x will increase coverage by 0.01%.
The diff coverage is 42.85%.

@@             Coverage Diff              @@
##              2.0.x     #555      +/-   ##
============================================
+ Coverage     70.51%   70.52%   +0.01%     
- Complexity      908      910       +2     
============================================
  Files           128      128              
  Lines          3646     3651       +5     
  Branches        256      258       +2     
============================================
+ Hits           2571     2575       +4     
+ Misses          943      941       -2     
- Partials        132      135       +3

Impacted Files	Coverage Δ	Complexity Δ
...mework/cloud/gateway/support/TimeoutException.java	`100% <ø> (+50%)`	`2 <0> (+1)`	⬆️
...ay/filter/factory/HystrixGatewayFilterFactory.java	`80.88% <42.85%> (-4.84%)`	`9 <0> (+1)`
...ler/predicate/RemoteAddrRoutePredicateFactory.java	`85.36% <0%> (+2.43%)`	`12% <0%> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b42c45f...0276702. Read the comment docs.

mmanciop · 2018-09-19T10:49:56Z

The failure looks like an unrelated failure, and for my local executions is broken also in master.

The tests affected by this change are fine though:

Running org.springframework.cloud.gateway.filter.factory.HystrixGatewayFilterFactoryTests Tests run: 7, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 11.927 sec

spencergibb · 2018-10-05T22:44:59Z

@mmanciop can you rebase? We're trying to release week after next.

mmanciop · 2018-10-06T06:07:46Z

On my machine the builds fail with an seemingly-unrelated redis rate-limit failure, but the Hystrix-related tests pass.

ryanjbaxter · 2018-10-06T14:44:37Z

Do you have a redis server running on you machine during your build?

mmanciop · 2018-10-06T15:09:15Z

No, I have not. This kind of the dependency would be however rather easy to automate with https://github.com/kstyrc/embedded-redis. I think I either have made a quick JUnit rule in the past for it, or used an existing one. I’ll consider contributing the solution to avoid this pitfall woth another PR.

ryanjbaxter · 2018-10-09T13:35:40Z

@mmanciop can you rebase?

mmanciop · 2018-10-09T14:37:19Z

Done. I found out that, due to #558, we have now another case to deal with; so I am wondering if in #558 we dealt with the mapping from TimeoutException to ResponseStatusException at too low a level.

Wouldn't it make sense to declare TimeoutException returning 504 to the client (e.g., by adding a @ResponseStatus annotation to it) and just let the TimeoutException bubble outside of the Hystrix command. Also, poking around about this, I got wondering if #558 broke the default settings of org.springframework.cloud.gateway.filter.factory.RetryGatewayFilterFactory.RetryConfig.

In any case, I believe it makes sense to let ResponseStatusException propagate unscathed out of a HystrixObservableCommand, as that is one of the premiere Spring ways to set status codes with applications; the other way I know being @ResponseStatus, for which we may likely need to add support here in case a filter (gateway or global, makes little difference) propagates one Throwable annotated with @ResponseStatus.

mmanciop · 2018-10-09T15:07:13Z

Meanwhile, I found out [1] that @ResponseStatus seems not to be supported by WebFlux, which I am not sure if it is a bug or a feature :-)

So I think this commit is good for the time being.

[1] https://github.com/mmanciop/response-status-repro.git

mmanciop · 2018-10-09T15:58:33Z

So, the Spring Cloud Gateway ignores @ResponseStatus but can deal with ResponseStatusException because the propagating exceptions are handled through org.springframework.boot.web.reactive.error.DefaultErrorAttributes#determineHttpStatus. @ryanjbaxter, @spencergibb, would it make sense to extend the DefaultErrorAttributes to deal with @ResponseStatus? I am frankly not so sure, as it may have side-effects when running a mix of plain WebFlux and Spring Cloud Gateway. I see it more as a PR to open upstream to spring-boot, so I'll give that a chance.

mmanciop · 2018-10-09T16:02:10Z

About @ResponseStatus, for reference, see spring-projects/spring-boot#14742

mmanciop · 2018-10-09T19:57:08Z

.../main/java/org/springframework/cloud/gateway/filter/factory/HystrixGatewayFilterFactory.java

+							Throwable cause = e.getCause();
+
+							if (cause instanceof ResponseStatusException) {
+								/*


When spring-projects/spring-boot#14742 is fixed upstream, here we should add support for @ResponseStatus using a logic similar to the one of WebFluxResponseStatusExceptionHandler

Fundamentally, the condition would change to:
cause instanceof ResponseStatusException || AnnotatedElementUtils.hasAnnotation(cause.getClass(), ResponseStatus.class)

mmanciop · 2018-10-09T20:02:15Z

.../main/java/org/springframework/cloud/gateway/filter/factory/HystrixGatewayFilterFactory.java

+								return Mono.error(cause);
+							}
+
+							if (cause instanceof TimeoutException) {


When spring-projects/spring-boot#14742 is fixed upstream, we can just add @ResponseStatus to TimeoutException and let the previous case (amended as per comment) take care of propagating the cause outside.

ryanjbaxter · 2018-10-11T13:42:12Z

Makes sense to me

mmanciop · 2018-10-11T15:24:35Z

As per spring-projects/spring-boot#14744, upstream should be fixed with 2.0.6. What is the timeline to update to a new Spring Boot Release after it becomes available?

mmanciop · 2018-10-12T05:12:54Z

@spencergibb I guess the label “waiting for feeback” should be removed :-)

ryanjbaxter · 2018-10-12T13:57:43Z

It looks like 2.0.6 is scheduled to be released on Oct 16th. There is a chance we might be able to do the Finchley.SR2 release based on that release. It all depends on when we get through the last few issues in our queue.

ryanjbaxter · 2018-10-17T19:48:05Z

@mmanciop we should now be using Boot 2.0.6 that was released today. We are planning on doing a release soon (probably by the end of the week), so if you update the PR soon to leverage @ResponseStatus we should be able to include it in the release.

mmanciop · 2018-10-18T05:56:26Z

The tests failing in this build are failing for me also for the tip of origin/2.0.x. Any pointers, @ryanjbaxter ?

ryanjbaxter · 2018-10-20T01:21:55Z

@mmanciop the build should be fixed, can you pull in the latest changes into your branch?

* Treat HttpClient timeouts the same as Hystrix timeouts * Propagate ResponseStatusException and throwables with @ResponseStatus when cause for COMMAND_EXECUTION failure Fix spring-cloud#554

mmanciop · 2018-10-20T08:04:08Z

@ryanjbaxter done ;-) Thanks for fixing the build.

mmanciop force-pushed the hystrix-webclient-timeouts branch from d41cd19 to 2419175 Compare September 19, 2018 10:48

mmanciop mentioned this pull request Sep 19, 2018

Make Gateway return 504 when HttpClient times out #558

Merged

mmanciop force-pushed the hystrix-webclient-timeouts branch from 2419175 to 6b1adda Compare October 1, 2018 08:52

spring-projects-issues added the waiting-for-triage label Oct 1, 2018

spencergibb changed the title ~~Treat WebClient timeouts the same as Hystrix timeouts~~ Treat HttpClient timeouts the same as Hystrix timeouts Oct 5, 2018

spencergibb removed the waiting-for-triage label Oct 5, 2018

spencergibb added the waiting for feedback label Oct 5, 2018

mmanciop force-pushed the hystrix-webclient-timeouts branch from 6b1adda to 444355e Compare October 6, 2018 06:06

mmanciop force-pushed the hystrix-webclient-timeouts branch from 444355e to 0f6d39c Compare October 9, 2018 14:27

mmanciop commented Oct 9, 2018

View reviewed changes

ryanjbaxter removed the waiting for feedback label Oct 12, 2018

ryanjbaxter added this to the 2.0.2.RELEASE milestone Oct 12, 2018

mmanciop force-pushed the hystrix-webclient-timeouts branch from 0f6d39c to be17f21 Compare October 17, 2018 21:53

mmanciop changed the base branch from master to 2.0.x October 17, 2018 21:57

mmanciop force-pushed the hystrix-webclient-timeouts branch from be17f21 to d6be3eb Compare October 17, 2018 22:13

Treat corner cases of Hystrix exception handling

0276702

* Treat HttpClient timeouts the same as Hystrix timeouts * Propagate ResponseStatusException and throwables with @ResponseStatus when cause for COMMAND_EXECUTION failure Fix spring-cloud#554

mmanciop force-pushed the hystrix-webclient-timeouts branch from d6be3eb to 0276702 Compare October 20, 2018 07:57

ryanjbaxter merged commit ded733b into spring-cloud:2.0.x Oct 22, 2018

mmanciop deleted the hystrix-webclient-timeouts branch October 22, 2018 16:34

Treat HttpClient timeouts the same as Hystrix timeouts #555

Treat HttpClient timeouts the same as Hystrix timeouts #555

Uh oh!

Conversation

mmanciop commented Sep 19, 2018

Uh oh!

codecov-io commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mmanciop commented Sep 19, 2018

Uh oh!

spencergibb commented Oct 5, 2018

Uh oh!

mmanciop commented Oct 6, 2018

Uh oh!

ryanjbaxter commented Oct 6, 2018

Uh oh!

mmanciop commented Oct 6, 2018

Uh oh!

ryanjbaxter commented Oct 9, 2018

Uh oh!

mmanciop commented Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmanciop commented Oct 9, 2018

Uh oh!

mmanciop commented Oct 9, 2018 • edited by ryanjbaxter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmanciop commented Oct 9, 2018

Uh oh!

mmanciop Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

mmanciop Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

mmanciop Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

ryanjbaxter commented Oct 11, 2018

Uh oh!

mmanciop commented Oct 11, 2018

Uh oh!

mmanciop commented Oct 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanjbaxter commented Oct 12, 2018

Uh oh!

ryanjbaxter commented Oct 17, 2018

Uh oh!

mmanciop commented Oct 18, 2018

Uh oh!

ryanjbaxter commented Oct 20, 2018

Uh oh!

mmanciop commented Oct 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Sep 19, 2018 •

edited

Loading

mmanciop commented Oct 9, 2018 •

edited

Loading

mmanciop commented Oct 9, 2018 •

edited by ryanjbaxter

Loading

mmanciop commented Oct 12, 2018 •

edited

Loading