HealthCheck causes calls to hang

[OlgaMaciaszek]

Bug can be reproduced in this sample:

https://github.com/OlgaMaciaszek/spring-cloud-example

Started after changes from:
6db6a81

[OlgaMaciaszek]

CC @robotmrv

[robotmrv]

@OlgaMaciaszek
Hi, the reason of hanging is CachingServiceInstanceListSupplier it tries to cache infinite Flux from HealthCheckServiceInstanceListSupplier and collect it to Mono<List<Signal<List<ServiceInstance>>>> see
https://github.com/spring-cloud/spring-cloud-commons/blob/master/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/CachingServiceInstanceListSupplier.java#L71

I think it is redundant as HealthCheckServiceInstanceListSupplier caches latest result

[spencergibb]

The point of CachingServiceInstanceListSupplier is caching.

[robotmrv]

to be more correct, Flux is collected into List in https://github.com/reactor/reactor-addons/blob/master/reactor-extra/src/main/java/reactor/cache/CacheFlux.java#L103

one more thing
There is no call of HealthCheckServiceInstanceListSupplier#afterPropertiesSet in
https://github.com/OlgaMaciaszek/spring-cloud-example/blob/fixed-config/microservice-b/src/main/java/com/example/microserviceb/MicroserviceBApplication.java#L47
which will allow caching on HealthCheckServiceInstanceListSupplier side

The point of CachingServiceInstanceListSupplier is caching.

yes, but you can't cache infinite stream, you need to stop at some time.
Flux is potentially infinite, so if you want to cache signals this way you need to get first List<ServiceInstance> in cache.

[robotmrv]

Btw, Flux infinity was discussed here. I understood that contract of ServiceInstanceListSupplier allows infinite streams

[spencergibb]

ok, so what is cached may need to be adjusted.

[OlgaMaciaszek]

one more thing
There is no call of HealthCheckServiceInstanceListSupplier#afterPropertiesSet

Yes, we would not usually expect the users to manually call afterPropertiesSet - I would move that back to constructor.

[OlgaMaciaszek]

Flux is potentially infinite, so if you want to cache signals this way you need to get first List<ServiceInstance> in cache.

Makes sense. Will probably need to adjust the CachingServiceInstanceListSupplier.

[robotmrv]

@OlgaMaciaszek what is wrong in making HealthCheckServiceInstanceListSupplier a bean?
And what about HealthCheckServiceInstanceListSupplier#destroy? if it is not a bean it should also be called manually

[OlgaMaciaszek]

That's a good point. We allow for tweaking the configs by creating hierarchies of delegates and we even have an easy way for users to do it via builders: see https://github.com/spring-cloud/spring-cloud-commons/blob/master/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliers.java, so we do not assume all delegates need to be beans, but as per @spencergibb suggestion, will change DelegatingServiceInstanceListSupplier (which will be the type of the highest level ServiceInstanceListSupplier that will always be a bean) to implement DisposableBean and if delegate is instance, call dispose(). Will move the code from afterPropertiesSet() to method called in constructor.