Skip to content

Conversation

@mmanciop
Copy link
Contributor

@mmanciop mmanciop commented Sep 19, 2018

When the underpinning Netty HttpClient times out (based, e.g., on the value of spring.cloud.gateway.httpclient.responseTimeout), the Gateway returns 500 to its client instead of 504.

This change wraps the org.springframework.cloud.gateway.support.TimeoutException thrown upon the timeout with a org.springframework.web.server.ResponseStatusException that sets the status code to 504.

As an added boon, using org.springframework.web.server.ResponseStatusException this way enables the usual error handling mechanisms (error white-page, error attributes) for this type of failure.

responseFlux = responseFlux.timeout(properties.getResponseTimeout(),
Mono.error(new TimeoutException("Response took longer than timeout: " +
properties.getResponseTimeout())));
properties.getResponseTimeout()))).onErrorMap(TimeoutException.class, th -> new ResponseStatusException(HttpStatus.GATEWAY_TIMEOUT, null, th));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, here one may say "Hey, why don't we get rid of TimeoutException altogether then"?

The answer is: it is a very convenient "marker" exception, which I already make use of in #555. I like the fact that, by not reusing java.util.concurrent.TimeoutException, one can tell apart this type of timeout from one issued, for example, by a filter using java.util.concurrent.Future for whatever dastardly reasons.

@codecov-io
Copy link

codecov-io commented Sep 19, 2018

Codecov Report

Merging #558 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #558      +/-   ##
============================================
+ Coverage     70.69%   70.69%   +<.01%     
  Complexity      930      930              
============================================
  Files           129      129              
  Lines          3672     3662      -10     
  Branches        263      263              
============================================
- Hits           2596     2589       -7     
+ Misses          938      935       -3     
  Partials        138      138
Impacted Files Coverage Δ Complexity Δ
...ework/cloud/gateway/filter/NettyRoutingFilter.java 94.73% <100%> (ø) 15 <1> (+1) ⬆️
...y/handler/predicate/PathRoutePredicateFactory.java 76.66% <0%> (-1.12%) 8% <0%> (ø)
...cloud/gateway/config/GatewayAutoConfiguration.java 76.31% <0%> (-0.41%) 59% <0%> (-1%)
...ork/cloud/gateway/route/builder/PredicateSpec.java 72.34% <0%> (+2.95%) 28% <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 2942df0...8257efd. Read the comment docs.

@mmanciop mmanciop changed the base branch from master to 2.0.x September 21, 2018 09:23
@mmanciop
Copy link
Contributor Author

Similarly to #557, I moved the PR to 2.x . Do we want to treat this as a breaking change and target only the next release (G... Grubsomething? :D). If we want to ship it only with the next release, I'll gladly move this back to master.

@mmanciop mmanciop changed the base branch from 2.0.x to master September 21, 2018 09:27
@mmanciop
Copy link
Contributor Author

Nevermind, it is a breaking change due to the change in status code. Well-behaving REST clients should not break because of a change from 500 to 504, but it would not be the first nor last time this happens in the wild.

@mmanciop mmanciop changed the title Make Gateway return 504 then HttpClient times out Make Gateway return 504 when HttpClient times out Oct 1, 2018
@spencergibb spencergibb added this to the 2.1.0.RC1 milestone Oct 5, 2018
@spencergibb spencergibb merged commit e626642 into spring-cloud:master Oct 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants