You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by "DeathGun44 (via GitHub)" <gi...@apache.org> on 2026/02/01 19:40:08 UTC
[PR] FINERACT-2449: Replace unbounded SimpleAsyncTaskExecutor with ThreadPoolTaskExecutor [fineract]
DeathGun44 opened a new pull request, #5432:
URL: https://github.com/apache/fineract/pull/5432
## Description
This PR addresses [FINERACT-2449](https://issues.apache.org/jira/browse/FINERACT-2449).
### Background
The current `SimpleAsyncTaskExecutor` creates a new thread for every task. While effective for light loads, this unbounded behavior poses a theoretical risk of thread exhaustion (`OutOfMemoryError: unable to create native thread`) under specific high-concurrency scenarios.
### Motivation
Relying on an unbounded executor is contrary to Spring Boot best practices for production-grade financial systems. This change proactively addresses the risk before it manifests in production.
### Solution
| Component | Change |
|-----------|--------|
| [SpringConfig.java](cci:7://file:///home/deathgun/fineract/fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/config/SpringConfig.java:0:0-0:0) | Replace `SimpleAsyncTaskExecutor` with `ThreadPoolTaskExecutor` |
| [FineractProperties.java](cci:7://file:///home/deathgun/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/core/config/FineractProperties.java:0:0-0:0) | Add `eventTaskExecutorCorePoolSize` and `eventTaskExecutorMaxPoolSize` |
| [application.properties](cci:7://file:///home/deathgun/fineract/fineract-provider/src/main/resources/application.properties:0:0-0:0) | Add externalized config with CPU-aware defaults |
### Testing
[SpringConfigTest.java](cci:7://file:///home/deathgun/fineract/fineract-provider/src/test/java/org/apache/fineract/infrastructure/core/config/SpringConfigTest.java:0:0-0:0) (Added 8 unit tests proving bounded behavior)
## 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).
- [ ] 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).
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2449: Replace unbounded SimpleAsyncTaskExecutor with ThreadPoolTaskExecutor [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #5432:
URL: https://github.com/apache/fineract/pull/5432#discussion_r2822897902
##########
fineract-provider/src/test/resources/application-test.properties:
##########
@@ -80,6 +80,8 @@ fineract.task-executor.tenant-upgrade-task-executor-core-pool-size=1
# This is intentionally restricted to a single thread due to an outstanding Liquibase thread-safety issue: https://github.com/liquibase/liquibase/pull/7227
fineract.task-executor.tenant-upgrade-task-executor-max-pool-size=1
fineract.task-executor.tenant-upgrade-task-executor-queue-capacity=${FINERACT_TENANT_UPGRADE_TASK_EXECUTOR_QUEUE_CAPACITY:100}
+fineract.task-executor.event-task-executor-core-pool-size=${FINERACT_EVENT_TASK_EXECUTOR_CORE_POOL_SIZE:0}
Review Comment:
Is this used anywhere?
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2449: Replace unbounded SimpleAsyncTaskExecutor with ThreadPoolTaskExecutor [fineract]
Posted by "DeathGun44 (via GitHub)" <gi...@apache.org>.
DeathGun44 commented on PR #5432:
URL: https://github.com/apache/fineract/pull/5432#issuecomment-3929251409
@adamsaghy This test failure seems unrelated to my changes
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2449: Replace unbounded SimpleAsyncTaskExecutor with ThreadPoolTaskExecutor [fineract]
Posted by "vidakovic (via GitHub)" <gi...@apache.org>.
vidakovic commented on PR #5432:
URL: https://github.com/apache/fineract/pull/5432#issuecomment-3832137466
I think there is a need to address this... but (having best practices in mind): let's first have a look here https://docs.spring.io/spring-boot/reference/features/task-execution-and-scheduling.html ... there are already (auto) configuration properties available, so why invent our own and create more Java configuration when they are not really necessary. I only cross-read that documentation quickly, but I think everything that is done here can be achieved with adding the proper "spring" prefixed properties to application.properties (with environment variable placeholders, sure); I think no extra/custom Java configuration would be needed.
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2449: Replace unbounded SimpleAsyncTaskExecutor with ThreadPoolTaskExecutor [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy merged PR #5432:
URL: https://github.com/apache/fineract/pull/5432
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2449: Replace unbounded SimpleAsyncTaskExecutor with ThreadPoolTaskExecutor [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #5432:
URL: https://github.com/apache/fineract/pull/5432#issuecomment-3883616440
> Hi @vidakovic, thanks for the review.
>
> I investigated using `spring.task.execution.*` as suggested. While those properties correctly auto-configure a TaskExecutor for `@Async` and related infrastructure, they don't fully address the requirement here for two reasons:
>
> **1. Synchronous by default** From the [Spring Boot docs](https://docs.spring.io/spring-boot/reference/features/task-execution-and-scheduling.html):
>
> > "The auto-configured AsyncTaskExecutor is used for: Execution of asynchronous tasks
> > using @EnableAsync, Spring MVC, Spring WebFlux, Spring WebSocket, JPA bootstrap..."
>
> `SimpleApplicationEventMulticaster` is not in that list. Per the [Spring Framework JavaDoc](https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/context/event/SimpleApplicationEventMulticaster.html#setTaskExecutor(java.util.concurrent.Executor)):
>
> > "Default is equivalent to SyncTaskExecutor, executing all listeners synchronously
> > in the calling thread."
>
> Unless `setTaskExecutor()` is explicitly called, event listeners run synchronously.
>
> **2. Security context propagation** Spring Boot's auto-configured executor is not wrapped with `DelegatingSecurityContextAsyncTaskExecutor`, nor is it associated with the event multicaster—both of which are required for correct async event handling in Fineract.
>
> Because of this, a properties-only solution isn't sufficient. That said, I'm happy to refactor the configuration to use Spring Boot's `ThreadPoolTaskExecutorBuilder`, so we can still honor the standard `spring.task.execution.*` pool sizing properties while explicitly wrapping the executor for security and wiring it into the multicaster.
>
> Does that approach work for you?
Sounds right to me.
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2449: Replace unbounded SimpleAsyncTaskExecutor with ThreadPoolTaskExecutor [fineract]
Posted by "vidakovic (via GitHub)" <gi...@apache.org>.
vidakovic commented on PR #5432:
URL: https://github.com/apache/fineract/pull/5432#issuecomment-3884354757
I learned again something... Thanks for digging this up. Suggestion sound good, let's then move on
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2449: Replace unbounded SimpleAsyncTaskExecutor with ThreadPoolTaskExecutor [fineract]
Posted by "DeathGun44 (via GitHub)" <gi...@apache.org>.
DeathGun44 commented on PR #5432:
URL: https://github.com/apache/fineract/pull/5432#issuecomment-3832702129
Hi @vidakovic, thanks for the review.
I investigated using `spring.task.execution.*` as suggested. While those properties
correctly auto-configure a [TaskExecutor](cci:2://file:///home/deathgun/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/core/config/FineractProperties.java:295:4-306:5) for `@Async` and related infrastructure,
they don't fully address the requirement here for two reasons:
**1. Synchronous by default**
From the [Spring Boot docs](https://docs.spring.io/spring-boot/reference/features/task-execution-and-scheduling.html):
> "The auto-configured AsyncTaskExecutor is used for: Execution of asynchronous tasks
> using @EnableAsync, Spring MVC, Spring WebFlux, Spring WebSocket, JPA bootstrap..."
`SimpleApplicationEventMulticaster` is not in that list. Per the
[Spring Framework JavaDoc](https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/context/event/SimpleApplicationEventMulticaster.html#setTaskExecutor(java.util.concurrent.Executor)):
> "Default is equivalent to SyncTaskExecutor, executing all listeners synchronously
> in the calling thread."
Unless `setTaskExecutor()` is explicitly called, event listeners run synchronously.
**2. Security context propagation**
Spring Boot's auto-configured executor is not wrapped with `DelegatingSecurityContextAsyncTaskExecutor`,
nor is it associated with the event multicaster—both of which are required for correct
async event handling in Fineract.
Because of this, a properties-only solution isn't sufficient. That said, I'm happy to
refactor the configuration to use Spring Boot's `ThreadPoolTaskExecutorBuilder`, so we
can still honor the standard `spring.task.execution.*` pool sizing properties while
explicitly wrapping the executor for security and wiring it into the multicaster.
Does that approach work for you?
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2449: Replace unbounded SimpleAsyncTaskExecutor with ThreadPoolTaskExecutor [fineract]
Posted by "DeathGun44 (via GitHub)" <gi...@apache.org>.
DeathGun44 commented on PR #5432:
URL: https://github.com/apache/fineract/pull/5432#issuecomment-3884551064
@vidakovic @adamsaghy Thank you for the feedbacks!
I've updated the PR with the discussed hybrid approach (Builder + Smart Defaults) and added tests proving the fix for thread exhaustion. Description is updated with full details ready for re-review when you have a moment!
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2449: Replace unbounded SimpleAsyncTaskExecutor with ThreadPoolTaskExecutor [fineract]
Posted by "DeathGun44 (via GitHub)" <gi...@apache.org>.
DeathGun44 commented on code in PR #5432:
URL: https://github.com/apache/fineract/pull/5432#discussion_r2823387801
##########
fineract-provider/src/test/resources/application-test.properties:
##########
@@ -80,6 +80,8 @@ fineract.task-executor.tenant-upgrade-task-executor-core-pool-size=1
# This is intentionally restricted to a single thread due to an outstanding Liquibase thread-safety issue: https://github.com/liquibase/liquibase/pull/7227
fineract.task-executor.tenant-upgrade-task-executor-max-pool-size=1
fineract.task-executor.tenant-upgrade-task-executor-queue-capacity=${FINERACT_TENANT_UPGRADE_TASK_EXECUTOR_QUEUE_CAPACITY:100}
+fineract.task-executor.event-task-executor-core-pool-size=${FINERACT_EVENT_TASK_EXECUTOR_CORE_POOL_SIZE:0}
Review Comment:
@adamsaghy you are correct, this is a leftover from my initial approach before I refactored to use spring.task.execution.* properties. I removed it now.
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2449: Replace unbounded SimpleAsyncTaskExecutor with ThreadPoolTaskExecutor [fineract]
Posted by "nikitoo0os (via GitHub)" <gi...@apache.org>.
nikitoo0os commented on PR #5432:
URL: https://github.com/apache/fineract/pull/5432#issuecomment-5072300161
Moving from SimpleAsyncTaskExecutor to a bounded ThreadPoolTaskExecutor solves the unbounded thread creation problem, but it also introduces another operational question: what is currently queued, running, rejected or stuck inside that pool?
I built GhostWork to provide task-level lifecycle visibility for Java and Spring executors:
https://github.com/nikitoo0os/ghostwork
It can complement the bounded executor configuration by exposing queue time, execution time, failures, cancellations and stuck tasks.
--
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