Skip to content

Conversation

@NYgomets
Copy link
Contributor

Description

This PR fixes a memory leak in CaffeineCacheManager where dynamically created caches were not removed when switching to static mode via setCacheNames().

Changes

Core Fix

  • Modified CaffeineCacheManager.setCacheNames() to clear dynamically created caches while preserving custom caches
  • Updated Javadoc to clarify the behavior

Tests Added

  • setCacheNamesShouldRemovePreviousDynamicCaches(): Verifies dynamic caches are cleared
  • setCacheNamesShouldPreserveCustomCaches(): Ensures custom caches are not affected
  • switchingBetweenDynamicAndStaticMode(): Tests mode transitions

Backward Compatibility

This is a potentially breaking change for users who rely on the (undocumented) behavior of dynamic caches persisting after setCacheNames().

However:

  1. The current behavior violates the documented contract
  2. The memory leak justifies the fix
  3. The typical use case (setting static caches at startup) is unaffected

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Nov 14, 2025
@NYgomets NYgomets closed this Nov 14, 2025
@NYgomets NYgomets reopened this Nov 14, 2025
@NYgomets NYgomets force-pushed the fix-cache-manager-static-mode-leak branch 5 times, most recently from b86be0c to 8e43a3f Compare November 14, 2025 17:37
When setCacheNames() is called to switch from dynamic to static mode,
dynamically created caches were not removed from the internal cacheMap,
causing:
1. Memory leak (orphaned cache references)
2. Violation of Javadoc contract stating cache names will be 'fixed'
3. getCacheNames() returning unexpected cache names

This fix ensures that dynamically created caches are cleared when
switching to static mode, while preserving custom caches registered
via registerCustomCache().

Signed-off-by: ParkJuhyeong <wngud5957@naver.com>
@NYgomets NYgomets force-pushed the fix-cache-manager-static-mode-leak branch from 8e43a3f to 47706be Compare November 14, 2025 17:52
@sbrannen sbrannen added the in: core Issues in core modules (aop, beans, core, context, expression) label Nov 15, 2025
@sbrannen sbrannen assigned sbrannen and jhoeller and unassigned sbrannen Nov 15, 2025
@jhoeller
Copy link
Contributor

jhoeller commented Nov 18, 2025

On review, setCacheNames is really a configuration-time method and not meant to be called at runtime when caches already exist. That said, point taken that it can effectively be used for re-configuration purposes as well, so we need to take that into account to some degree.

For the specific case of "leaking" dynamic cache regions when a new set of fixed cache names is being configured, these formerly registered dynamic regions keep being accessible, so they are not really leaking. Also, if a specific set of cache names is set through setCacheNames initially, and then later on a smaller or different set of cache names is set on yet another setCacheNames call, we are not removing the old caches either, rather just adding the newly specified ones. The registerCustomCache facility makes the semantics even less clear there since those are always worth retaining.

Overall, I don't see us adding official clean-up semantics to setCacheNames. Instead, we're introducing a resetCaches() method (#35840) which can be called before setCacheNames if such clean-up is desirable: That method can generally be used for cache clearing at runtime, removing dynamic caches or simply clearing static caches. This goes nicely with the existing removeCache method that we introduced in 6.1.15 (#33813).

Thanks for the PR, in any case!

@jhoeller jhoeller closed this Nov 18, 2025
@jhoeller jhoeller added status: superseded An issue that has been superseded by another and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Nov 18, 2025
@NYgomets
Copy link
Contributor Author

Thanks for the detailed explanation, Juergen.

I understand the design philosophy regarding setCacheNames as a configuration-time method and the risk of implicit cleanup. Introducing an explicit resetCaches() method seems like a much safer and more flexible approach to handle runtime reconfiguration scenarios.

I'm glad that my report contributed to improving the cache management capabilities. Thanks for addressing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

in: core Issues in core modules (aop, beans, core, context, expression) status: superseded An issue that has been superseded by another

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants