spring-cloud/spring-cloud-commons

The use of boundedElastic Scheduler in blocking DiscoveryClientServiceInstanceListSupplier

longkunbetter opened this issue · 2 comments

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.

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

@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.