Skip to content

Commit 47c6f1c

Browse files
authored
refactor: fix reactive anti-pattern and cache key in CachedSitemapGetter (#47)
Replace Guava Cache with Caffeine AsyncCache to eliminate the blocking .block() call inside a reactive pipeline, which can cause thread starvation on bounded schedulers. Use options.getSiteUrl().toString() as the cache key instead of the SitemapGeneratorOptions object itself. java.net.URL.equals() performs DNS resolution on every comparison, making it unsuitable as a map/cache key. Made-with: Cursor
1 parent 74b3475 commit 47c6f1c

2 files changed

Lines changed: 26 additions & 30 deletions

File tree

Lines changed: 15 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,39 +1,33 @@
11
package run.halo.sitemap;
22

3-
import com.google.common.cache.Cache;
4-
import com.google.common.cache.CacheBuilder;
3+
import com.github.benmanes.caffeine.cache.AsyncCache;
4+
import com.github.benmanes.caffeine.cache.Caffeine;
55
import java.time.Duration;
66
import lombok.AllArgsConstructor;
7-
import org.apache.commons.lang3.StringUtils;
87
import org.springframework.stereotype.Component;
98
import reactor.core.publisher.Mono;
10-
import reactor.core.scheduler.Schedulers;
119

1210
@Component
1311
@AllArgsConstructor
1412
public class CachedSitemapGetter {
1513

16-
private final Cache<SitemapGeneratorOptions, String> cache = CacheBuilder.newBuilder()
17-
.concurrencyLevel(Runtime.getRuntime().availableProcessors())
18-
.initialCapacity(8)
19-
.maximumSize(8)
14+
private final AsyncCache<String, String> cache = Caffeine.newBuilder()
2015
.expireAfterWrite(Duration.ofSeconds(30))
21-
.build();
16+
.maximumSize(8)
17+
.buildAsync();
2218

2319
private final DefaultSitemapEntryLister lister;
2420

2521
public Mono<String> get(SitemapGeneratorOptions options) {
26-
return Mono.fromCallable(() -> cache.get(options, () -> lister.list(options)
27-
.collectList()
28-
.map(entries -> {
29-
String xml = new SitemapBuilder()
30-
.buildSitemapXml(entries);
31-
cache.put(options, xml);
32-
return xml;
33-
})
34-
.defaultIfEmpty(StringUtils.EMPTY)
35-
.block()
36-
))
37-
.subscribeOn(Schedulers.boundedElastic());
22+
String key = options.getSiteUrl().toString();
23+
return Mono.fromFuture(() ->
24+
cache.get(key, (k, executor) ->
25+
lister.list(options)
26+
.collectList()
27+
.map(entries -> new SitemapBuilder().buildSitemapXml(entries))
28+
.defaultIfEmpty("")
29+
.toFuture()
30+
)
31+
);
3832
}
3933
}

src/test/java/run/halo/sitemap/CachedSitemapGetterTest.java

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
package run.halo.sitemap;
22

33
import static org.mockito.ArgumentMatchers.any;
4-
import static org.mockito.Mockito.mock;
4+
import static org.mockito.Mockito.times;
55
import static org.mockito.Mockito.verify;
66
import static org.mockito.Mockito.when;
77

8+
import java.net.MalformedURLException;
9+
import java.net.URL;
810
import java.util.List;
911
import java.util.concurrent.ExecutionException;
1012
import java.util.concurrent.Executors;
@@ -22,33 +24,33 @@ public class CachedSitemapGetterTest {
2224
@Mock
2325
private DefaultSitemapEntryLister lister;
2426
private CachedSitemapGetter getter;
27+
private SitemapGeneratorOptions options;
2528

2629
@BeforeEach
27-
void setUp() {
30+
void setUp() throws MalformedURLException {
2831
when(lister.list(any())).thenReturn(
2932
Flux.just(SitemapEntry.builder().loc("http://localhost:8090/about").build()));
3033
getter = new CachedSitemapGetter(lister);
34+
options = SitemapGeneratorOptions.builder()
35+
.siteUrl(new URL("http://localhost:8090"))
36+
.build();
3137
}
3238

3339
@Test
3440
void get() throws InterruptedException, ExecutionException {
35-
var options = mock(SitemapGeneratorOptions.class);
36-
3741
getter.get(options).block();
38-
verify(lister).list(options);
42+
verify(lister).list(any());
3943

4044
var executorService = Executors.newCachedThreadPool();
4145

4246
List<? extends Future<?>> futures = IntStream.range(0, 10)
43-
.mapToObj(i -> executorService.submit(() -> {
44-
getter.get(options).block();
45-
}))
47+
.mapToObj(i -> executorService.submit(() -> getter.get(options).block()))
4648
.toList();
4749

4850
for (Future<?> future : futures) {
4951
future.get();
5052
}
5153

52-
verify(lister).list(options);
54+
verify(lister, times(1)).list(any());
5355
}
5456
}

0 commit comments

Comments
 (0)