Skip to content

refactor: fix reactive anti-pattern and cache key in CachedSitemapGetter#47

Merged
ruibaby merged 1 commit intohalo-dev:mainfrom
ruibaby:refactor/cached-sitemap-getter
Apr 27, 2026
Merged

refactor: fix reactive anti-pattern and cache key in CachedSitemapGetter#47
ruibaby merged 1 commit intohalo-dev:mainfrom
ruibaby:refactor/cached-sitemap-getter

Conversation

@ruibaby
Copy link
Copy Markdown
Member

@ruibaby ruibaby commented Apr 27, 2026

Summary

Three issues in the previous CachedSitemapGetter implementation:

  1. Reactive anti-pattern: Guava Cache.get(key, Callable) was used with .block() inside the callable. This is a reactive anti-pattern — even on Schedulers.boundedElastic(), nesting .block() inside a Guava cache loader risks deadlock under concurrent load.
  2. DNS-triggering cache key: SitemapGeneratorOptions (which contains java.net.URL) was used as the cache key. URL.equals() and URL.hashCode() perform DNS resolution on every cache lookup, which is a well-known Java gotcha.
  3. Redundant cache.put(): Guava's cache.get(key, callable) automatically stores the callable result — the explicit cache.put() call afterward was redundant.

Changes

  • Replace Guava Cache with Caffeine AsyncCache<String, String>
  • Use siteUrl.toString() as the cache key (plain String, no DNS lookup)
  • The loader returns a CompletableFuture directly from the reactive pipeline (.toFuture()), wrapped in Mono.fromFuture() — no .block() anywhere
  • Caffeine automatically deduplicates concurrent loads for the same key

Test plan

  • CachedSitemapGetterTest.get() — lister is called exactly once even with 10 concurrent requests hitting the same key
  • Verify no blocking calls appear on the reactive event loop thread

Made with Cursor

None

@f2c-ci-robot f2c-ci-robot Bot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Apr 27, 2026
@f2c-ci-robot
Copy link
Copy Markdown

f2c-ci-robot Bot commented Apr 27, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign guqing for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@f2c-ci-robot f2c-ci-robot Bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 27, 2026
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
@ruibaby ruibaby force-pushed the refactor/cached-sitemap-getter branch from 6ebf3d0 to 1bd60c9 Compare April 27, 2026 08:53
@f2c-ci-robot f2c-ci-robot Bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 27, 2026
@ruibaby ruibaby merged commit 47c6f1c into halo-dev:main Apr 27, 2026
1 of 2 checks passed
@ruibaby ruibaby deleted the refactor/cached-sitemap-getter branch April 27, 2026 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note-none Denotes a PR that doesn't merit a release note.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant