search

spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.75k stars 38.15k forks source link

Deprecate exchangeTimeout and refactor readTimeout in ReactorClientHttpRequestFactory #33782

Closed rstoyanchev closed 4 weeks ago

rstoyanchev commented 1 month ago

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.

bclozel commented 1 month ago

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 commented 1 month ago

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

rstoyanchev commented 4 weeks ago

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.