Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate exchangeTimeout and refactor readTimeout in ReactorClientHttpRequestFactory #33782

Closed
rstoyanchev opened this issue Oct 24, 2024 · 3 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Milestone

Comments

@rstoyanchev
Copy link
Contributor

The exchangeTimeout and readTimeout are used where it's necessary to call Mono.block() to avoid indefinite blocking, but Reactor Netty has a variety of timeout configuration options underneath, and such higher level timeouts get in the way, if they are shorter, creating a confusing out of the box experience and overall configuration model.

The point of blocking is not necessarily the best place to set up timeouts. The exchangeTimeout timeout for example is not the full exchange as it sounds, but only to the point of getting the ClientHttpResponse. The readTimeout blocks on the aggregation of the response, the response shouldn't be aggregated, see #33781, and more importantly Reactor Netty also has readTimeout options that aren't the same thing, it's confusing for use to re-use the same name.

Rather than add timeout options, we should rely on the underlying client to provide that. It is in a much better position to do so, and for anything missing, it's better to add that in Reactor Netty. At best we should help to configure options on the underlying client, like we do for connectTimeout.

@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug labels Oct 24, 2024
@rstoyanchev rstoyanchev added this to the 6.2.0 milestone Oct 24, 2024
@rstoyanchev rstoyanchev self-assigned this Oct 24, 2024
@bclozel
Copy link
Member

bclozel commented Oct 24, 2024

I think some of those are being used by Spring Boot to configure HTTP clients in a consistent manner. See https://github.com/spring-projects/spring-boot/blob/main/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/client/ClientHttpRequestFactories.java#L259-L260

Maybe we can keep those around but call the native Reactor Netty methods to improve the behavior?

@rstoyanchev
Copy link
Contributor Author

rstoyanchev commented Oct 24, 2024

Good point. Maybe we'll keep the readTimeout, and map it to the most suitable Reactor Netty config option.

@rstoyanchev rstoyanchev changed the title Deprecate readTimeout and exchangeTimeout in ReactorClientHttpRequestFactory Deprecate exchangeTimeout and refactor readTimeout in ReactorClientHttpRequestFactory Oct 28, 2024
@rstoyanchev
Copy link
Contributor Author

rstoyanchev commented Oct 28, 2024

readTimeout is no longer applicable (as it was implemented) after #33781 since ReactorClientHttpResponse neither aggregates nor blocks. Instead it is mapped to HttpClient#responseTimeout, providing consistent behavior for connect and read timeout properties in Spring Boot, which for all other clients map to underlying client properties. The main difference is vs before is that readTimeout now determined the maximum duration between reads rather than the reading of the full response.

exchangeTimeout is deprecated in favor of Reactor Netty timeout configuration. It is still applied during the deprecation phase, but the 5 second default is dropped. Given that Reactor Netty has default timeout values for connect, SSE, proxy, and host name resolution, and given that we set responseTimeout by default, there should not be indefinite hanging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Projects
None yet
Development

No branches or pull requests

2 participants