spring-cloud / spring-cloud-commons

Common classes used in different Spring Cloud implementations
Apache License 2.0
707 stars 704 forks source link

The use of boundedElastic Scheduler in blocking DiscoveryClientServiceInstanceListSupplier #1298

Closed longkunbetter closed 9 months ago

longkunbetter commented 11 months ago

Is your feature request related to a problem? Please describe. the related codes are in this class spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/DiscoveryClientServiceInstanceListSupplier.java

It use timeout(Duration timeout,@Nullable Publisher<? extends T> fallback,Scheduler timer) to implement a logic that will return a list of server instance in a reactor style.

But this will introduce a new thread for each request, does that voilate the 'less thread' ideaology of reactor?

Describe the solution you'd like Maybe use a Sink API can avoid the thread creation. Describe alternatives you've considered eg. Let the Discovery Client to maintain a group of loop threads to handle the fetch service instances logic.

OlgaMaciaszek commented 9 months ago

Hello, @longkunbetter. Thanks for reporting the issue. We'll provide a fix.

OlgaMaciaszek commented 9 months ago

@longkunbetter upon further analysis, the timeout variant with Scheduler as an argument is used in case of a blocking DiscoveryClient, for which retries could cause blocking on a non-blocking Thread, that’s why Schedulers.boundedElastic is used. The behaviour of it is to manage a bounded amount of Threads, limited by default to 10 * numberOfCpus and only happens if there actually is a timeout. What seems like an omission is that the initial request might happen on a non-blocking reactive Thread, therefore we can correct it to subscribe on the boundedElastic Scheduler as well, however, since it'd cause possibly unnecessary thread switching in the implementation that is anyhow blocking, we won't be doing it at this point.