CachingRouteLocator.updateCache does not work as intended (synchronized method performs async subscribe)

[ririnto]

Describe the bug

Spring Cloud Version: 4.3.0
Module: spring-cloud-gateway-server-webflux

CachingRouteLocator.updateCache is declared synchronized, but the cache refresh is triggered via an asynchronous subscribe() on a Flux<Route> (from onApplicationEvent), so the actual cache population completes after the synchronized method returns. This means the synchronized boundary does not protect the real update.

Under concurrent or back-to-back refreshes, an older (slower) refresh can finish after a newer (faster) one and persist a stale/empty cache, causing the cache to regress. For example, when a new route appears, a slow refresh that observed “no routes yet” may complete after a fast refresh that already saw the new route and then overwrite the cache with an older/empty state.

Note: Finishing later doesn’t guarantee newer data. When refreshes overlap, you may need to run updateCache again or ensure only the newest snapshot is kept (e.g., sequence/timestamp).

Sample

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.springframework.cloud.gateway.event.RefreshRoutesEvent;
import org.springframework.cloud.gateway.event.RefreshRoutesResultEvent;
import org.springframework.cloud.gateway.route.CachingRouteLocator;
import org.springframework.cloud.gateway.route.Route;
import org.springframework.cloud.gateway.route.RouteLocator;
import org.springframework.context.ApplicationEventPublisher;
import reactor.core.publisher.Flux;

import java.time.Duration;
import java.util.List;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;

import static org.assertj.core.api.Assertions.assertThat;

public class CachingRouteLocatorEmptyCacheRaceTest {
    @DisplayName("latest refresh should win when a new route appears")
    @Test
    void shouldUpdateCache() throws Exception {
        Route newRoute = Route.async()
                .id("new")
                .uri("http://localhost/new")
                .order(0)
                .predicate(exchange -> true)
                .build();

        RouteLocator delegate = new RouteLocator() {
            int calls = 0;

            @Override
            public Flux<Route> getRoutes() {
                int n = calls++;
                if (n == 0) {
                    return Flux.<Route>empty().delaySubscription(Duration.ofMillis(600));
                }
                return Flux.just(newRoute).delaySubscription(Duration.ofMillis(40));
            }
        };

        CachingRouteLocator locator = new CachingRouteLocator(delegate);

        CountDownLatch cdl = new CountDownLatch(2);
        ApplicationEventPublisher publisher = event -> {
            if (event instanceof RefreshRoutesResultEvent) {
                cdl.countDown();
            }
        };
        locator.setApplicationEventPublisher(publisher);

        locator.onApplicationEvent(new RefreshRoutesEvent(this));
        locator.onApplicationEvent(new RefreshRoutesEvent(this));

        boolean finished = cdl.await(3, TimeUnit.SECONDS);
        assertThat(finished)
                .withFailMessage("Refresh did not complete within the timeout; adjust delays or timeout to proceed.")
                .isTrue();

        List<Route> finalRoutes = locator.getRoutes().collectList().block(Duration.ofSeconds(2));
        assertThat(finalRoutes)
                .withFailMessage("Cache should reflect the latest refresh result (non-empty expected when a new route appears). Actual: %s", finalRoutes)
                .isNotEmpty();
    }
}