You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fineract.apache.org by "pranavdhanabalan (via GitHub)" <gi...@apache.org> on 2026/09/09 16:40:40 UTC
[PR] FINERACT-2822: Prevent concurrent modification of supported cache names [fineract]
pranavdhanabalan opened a new pull request, #6424:
URL: https://github.com/apache/fineract/pull/6424
## Description
Prevent concurrent modification of `supportedCacheNames` in
`SpecifiedCacheSupportingCacheManager`.
Synchronize access to the internal `LinkedHashSet` and return a snapshot
from `getCacheNames()` to prevent callers from iterating over the internal
collection while it is being modified.
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [x] Write the commit message as per [our guidelines](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#pull-requests)
- [x] Acknowledge that we will not review PRs that are not passing the build _("green")_ - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
- [x] Create/update [unit or integration tests](https://fineract.apache.org/docs/current/#_testing) for verifying the changes made.
- [x] Follow our [coding conventions](https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions).
- [x] Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
- [x] [This PR must not be a "code dump"](https://cwiki.apache.org/confluence/display/FINERACT/Pull+Request+Size+Limit). Large changes can be made in a branch, with assistance. Ask for help on the [developer mailing list](https://fineract.apache.org/#contribute).
- [x] If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.
- [x] I followed the [AI Policy](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#ai-policy).
Your assigned reviewer(s) will follow our [guidelines for code reviews](https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2822: Prevent concurrent modification of supported cache names [fineract]
Posted by "pranavdhanabalan (via GitHub)" <gi...@apache.org>.
pranavdhanabalan commented on PR #6424:
URL: https://github.com/apache/fineract/pull/6424#issuecomment-5686983112
I’ve squashed the changes into a single commit and updated the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2822: Prevent concurrent modification of supported cache names [fineract]
Posted by "vidakovic (via GitHub)" <gi...@apache.org>.
vidakovic commented on code in PR #6424:
URL: https://github.com/apache/fineract/pull/6424#discussion_r4023416216
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/config/cache/SpecifiedCacheSupportingCacheManager.java:
##########
@@ -77,6 +80,8 @@ public void setNoOpCacheManager(NoOpCacheManager noOpCacheManager) {
}
public void setSupportedCaches(String... cacheNames) {
- supportedCacheNames.addAll(Arrays.asList(cacheNames));
+ synchronized (supportedCacheNames) {
Review Comment:
This function is only called from a Spring Java configuration class. How is this supposed to create a concurrency issue?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2822: Prevent concurrent modification of supported cache names [fineract]
Posted by "vidakovic (via GitHub)" <gi...@apache.org>.
vidakovic commented on code in PR #6424:
URL: https://github.com/apache/fineract/pull/6424#discussion_r4023416216
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/config/cache/SpecifiedCacheSupportingCacheManager.java:
##########
@@ -77,6 +80,8 @@ public void setNoOpCacheManager(NoOpCacheManager noOpCacheManager) {
}
public void setSupportedCaches(String... cacheNames) {
- supportedCacheNames.addAll(Arrays.asList(cacheNames));
+ synchronized (supportedCacheNames) {
Review Comment:
This function is only called from a Spring Java configuration class. How is this supposed to create a concurrency issue? This looks like a solution to a non-existing problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2822: Prevent concurrent modification of supported cache names [fineract]
Posted by "vidakovic (via GitHub)" <gi...@apache.org>.
vidakovic commented on code in PR #6424:
URL: https://github.com/apache/fineract/pull/6424#discussion_r4023388369
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/config/cache/SpecifiedCacheSupportingCacheManager.java:
##########
@@ -64,7 +67,7 @@ public Cache getCache(String name) {
@Override
public Collection<String> getCacheNames() {
synchronized (supportedCacheNames) {
- return Collections.unmodifiableSet(supportedCacheNames);
+ return new LinkedHashSet<>(supportedCacheNames);
Review Comment:
What's the goal/advantage of making this a modifiable collection?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2822: Prevent concurrent modification of supported cache names [fineract]
Posted by "vidakovic (via GitHub)" <gi...@apache.org>.
vidakovic commented on code in PR #6424:
URL: https://github.com/apache/fineract/pull/6424#discussion_r4023383718
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/config/cache/SpecifiedCacheSupportingCacheManager.java:
##########
@@ -49,7 +48,11 @@ public void afterPropertiesSet() throws Exception {
@Override
public Cache getCache(String name) {
- if (supportedCacheNames.contains(name)) {
+ boolean contains;
+ synchronized (supportedCacheNames) {
Review Comment:
Is it really necessary to have a `synchronized` block here? Are the cache names actually changing after boot? If we really have a concurrency issue here then why not ReentrantLock?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2822: Prevent concurrent modification of supported cache names [fineract]
Posted by "vidakovic (via GitHub)" <gi...@apache.org>.
vidakovic commented on code in PR #6424:
URL: https://github.com/apache/fineract/pull/6424#discussion_r4023447109
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/config/cache/SpecifiedCacheSupportingCacheManager.java:
##########
@@ -49,7 +48,11 @@ public void afterPropertiesSet() throws Exception {
@Override
public Cache getCache(String name) {
- if (supportedCacheNames.contains(name)) {
+ boolean contains;
+ synchronized (supportedCacheNames) {
Review Comment:
I checked again, the only place where the cache names change is when `setSupportedCaches` is called... and that happens only once during startup of the application. From what I see the `synchronized` makes not much sense here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2822: Prevent concurrent modification of supported cache names [fineract]
Posted by "pranavdhanabalan (via GitHub)" <gi...@apache.org>.
pranavdhanabalan closed pull request #6424: FINERACT-2822: Prevent concurrent modification of supported cache names
URL: https://github.com/apache/fineract/pull/6424
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2822: Prevent concurrent modification of supported cache names [fineract]
Posted by "AshharAhmadKhan (via GitHub)" <gi...@apache.org>.
AshharAhmadKhan commented on PR #6424:
URL: https://github.com/apache/fineract/pull/6424#issuecomment-5686494433
Single commit per PR please.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2822: Prevent concurrent modification of supported cache names [fineract]
Posted by "pranavdhanabalan (via GitHub)" <gi...@apache.org>.
pranavdhanabalan commented on PR #6424:
URL: https://github.com/apache/fineract/pull/6424#issuecomment-5694208104
Hello Sir,
Thanks for pointing this out,
I originally added the synchronization while addressing the SpotBugs IS2_INCONSISTENT_SYNC warning, since SpotBugs was reporting inconsistent synchronization around supportedCacheNames.
After checking the lifecycle of supportedCacheNames, I confirmed that setSupportedCaches() is only called during Spring startup and the cache names are not modified afterward. I agree that synchronization is unnecessary here, so I'll remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2822: Prevent concurrent modification of supported cache names [fineract]
Posted by "vidakovic (via GitHub)" <gi...@apache.org>.
vidakovic commented on code in PR #6424:
URL: https://github.com/apache/fineract/pull/6424#discussion_r4023388369
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/config/cache/SpecifiedCacheSupportingCacheManager.java:
##########
@@ -64,7 +67,7 @@ public Cache getCache(String name) {
@Override
public Collection<String> getCacheNames() {
synchronized (supportedCacheNames) {
- return Collections.unmodifiableSet(supportedCacheNames);
+ return new LinkedHashSet<>(supportedCacheNames);
Review Comment:
What's the goal/advantage of making this a modifiable collection? The underlying collection is already a `LinkedHashSet`... really not understanding what you intend to solve here. BTW: as mentioned before, I think the `synchronized` is unnecessary here. After app startup we don't have to seem any changes in that collection. If we want to be sure then we should make this collection with the cache names immediately unmodifiable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org